Replace application list view with a Vue ViewModel #402

martinRenou · 2017-04-18T17:02:57Z

No description provided.

stefanoborini · 2017-04-24T15:57:38Z

remoteappmanager/static/js/home/controller.js

    // This model keeps the retrieved content from the REST query locally.
    // It is only synchronized at initial load.
    var model = new models.ApplicationListModel();
-    var app_list_view = new application_list_view.ApplicationListView(model);
+    var app_list_view = application_list_view.applicationListView;


use underscores.

Also assign the model at this stage.

stefanoborini · 2017-04-24T16:06:22Z

remoteappmanager/templates/home.html

+      We initialize this element with "display: none" to prevent displaying
+      "No applications found" and the loading spinner at the same time
+    -->
+    <li v-show="!loading && model.app_list.length === 0" style="display: none;">


Try to use a v-else.

There's no gain to use v-if and v-else instead of two v-show in terms of lines of code. And my problem is that it changes the behavior of the dom elements, v-ifwill result in rendering the element only if the statement is true, v-showonly changes the style attribute displayof the element, so I have to change the selenium tests which relies on the style attribute

stefanoborini · 2017-04-24T16:06:57Z

remoteappmanager/templates/home.html

+    -->
+    <li v-for="(app, index) in model.app_list"
+        v-bind:class="{ active: index === model.selected_index }"
+        v-on:click="model.selected_index = index"


v-on -> @click

stefanoborini · 2017-04-24T16:07:22Z

remoteappmanager/templates/home.html

+      issues when the javascript is not loaded yet (temporary ?)
+    -->
+    <li v-for="(app, index) in model.app_list"
+        v-bind:class="{ active: index === model.selected_index }"


v-bind also has a shorthand. I think it's just :class

stefanoborini · 2017-04-24T16:10:02Z

remoteappmanager/templates/home.html

+             v-bind:src="get_icon_src(app.app_data)">
+
+        <button class="stop-button"
+                v-show="app.status === 'RUNNING' && index_mouse_over === index"


Try to do this with simple CSS

stefanoborini · 2017-04-24T16:40:29Z

remoteappmanager/static/js/home/views/application_list_view.js

+            stop_application: function(index) {
+                this.stop_application_callback(index);
+            },
+            mouse_enter: function(index) {


maybe as CSS?

stefanoborini · 2017-04-24T16:41:03Z

remoteappmanager/static/js/home/views/application_list_view.js

+                    this.index_mouse_over = index;
+                }
+            },
+            mouse_leave: function() {


maybe with CSS?

stefanoborini · 2017-04-24T16:42:01Z

remoteappmanager/static/js/home/views/application_list_view.js

-        if (self.model.selected_index === index) {
-            self.update_selected();
+        watch: {
+            'model.selected_index': function() { this.selected_app_callback(); }


This will eventually go away when the syncronization is performed through the model

stefanoborini · 2017-04-24T16:51:41Z

remoteappmanager/static/js/home/views/application_view.js

        if (properties.length === 0) {
            fieldset.html("No configurable options for this image");
        } else {
            properties.forEach(
                function(val) {  // jshint ignore:line
-                    var widget = configurables[val].view(index);
-                    fieldset.append(widget);
+                    // Vue.js adds an __ob__ property, we have to fix it


If you replace the properties with a list, it should cause no harm and solve this.

stefanoborini · 2017-04-25T08:26:54Z

bower.json

@@ -1,32 +1,33 @@
 {
-    "name": "simphony-remote-deps",


Use four spaces for indentation. I checked the preferred style guide, and most of the opinions are for four spaces, like in python, including Crockford. We don't have to save space, minification will do that (if ever, js files are cached anyway)

Ok, correction. Google uses two. uhm....

ok, keep two.

Better, keep four for now, and we'll switch to two after this PR is in

stefanoborini · 2017-04-25T08:39:09Z

bower.json

@@ -1,32 +1,33 @@
 {
-    "name": "simphony-remote-deps",


Better, keep four for now, and we'll switch to two after this PR is in

…property

codecov-io · 2017-04-25T11:24:27Z

Codecov Report

Merging #402 into switch_to_vue will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           switch_to_vue     #402   +/-   ##
==============================================
  Coverage          95.17%   95.17%           
==============================================
  Files                 94       94           
  Lines               3996     3996           
  Branches             256      256           
==============================================
  Hits                3803     3803           
  Misses               139      139           
  Partials              54       54

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c892b79...a8d6dfb. Read the comment docs.

The gain of the last changes was null and it results in test failing. I prefer to undo those changes instead of changing the tests.

good

martinRenou and others added 24 commits April 18, 2017 16:44

Install vue with bower instead of NPM

87df38e

Replace the old ApplicationListView by a Vue ViewModel

d7a09a7

Delete unused handlebars helpers

4c08083

Create ApplicationListView template

14fc736

Fixed imports

051d193

Fix import

b25d692

Started to adapt the controller to the new Application list ViewModel

60ffdfb

Changed data representation of the model

1cbd133

Adapt controller with the new model data representation

b625942

Adapt ApplicationListView with the new model data representation

a9cf5a0

Adapt ApplicationView with the new model data representation

0440ef5

Fixed little issues

31ddfaf

Add comment

27928b2

Add stop button

6760912

Fix linter tests

e3f334e

Fix model tests

c17ef68

Fix view tests

5554651

Try to fix selenium test (DOM elements has been changed)

88b2ca1

Use minified version of Vue

7143e94

Try to fix selenium tests

adee7bd

Fix selenium tests

c5891a3

Fix selenium test

7c82057

Fix selenium tests

08c22a7

Fixed init path

ed7ae38

martinRenou requested a review from stefanoborini April 25, 2017 08:27

stefanoborini reviewed Apr 25, 2017

View reviewed changes

stefanoborini previously requested changes Apr 25, 2017

View reviewed changes

martinRenou added 3 commits April 25, 2017 09:42

Fix indentation of the bower file

3acffe7

Now assign the model to the ApplicationListView at instantiation

d4c982c

Use shorthands for vue attributes

a15b9d3

martinRenou added 6 commits April 25, 2017 10:55

The stop-button hover event is now triggered with pure CSS

5d5ccd9

Clear unused code

93a48cd

Use filters for icon and application name

48eb4a8

Configurables is now a list, fixing the issue of Vue adding a __ob__ …

0c35794

…property

Try to fix focus selenium test

11eaf35

Fix jstests

3f52280

martinRenou added 3 commits April 25, 2017 12:34

Use if else-if

88e398f

Undo last commit

1bea069

The gain of the last changes was null and it results in test failing. I prefer to undo those changes instead of changing the tests.

Fix issue with unknown "this"

a8d6dfb

martinRenou merged commit 4e5c077 into switch_to_vue Apr 25, 2017

martinRenou changed the title ~~Replace application list view with a Vue ViewModel [WIP]~~ Replace application list view with a Vue ViewModel Apr 25, 2017

martinRenou deleted the replace_application_list2 branch April 25, 2017 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace application list view with a Vue ViewModel #402

Replace application list view with a Vue ViewModel #402

martinRenou commented Apr 18, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

martinRenou Apr 25, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 24, 2017

stefanoborini Apr 25, 2017

stefanoborini Apr 25, 2017

stefanoborini Apr 25, 2017

stefanoborini Apr 25, 2017

stefanoborini Apr 25, 2017

codecov-io commented Apr 25, 2017 •

edited

Loading

Replace application list view with a Vue ViewModel #402

Replace application list view with a Vue ViewModel #402

Conversation

martinRenou commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 25, 2017 • edited Loading

Codecov Report

codecov-io commented Apr 25, 2017 •

edited

Loading