Resolution configurable is now a Vue component #420

martinRenou · 2017-05-04T10:08:54Z

No description provided.

Just define a description of each configurable for each application.

And fixed the issue of config_dict not correctly set by default

…erty

codecov-io · 2017-05-04T14:09:42Z

Codecov Report

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

@@              Coverage Diff               @@
##           switch_to_vue     #420   +/-   ##
==============================================
  Coverage          95.33%   95.33%           
==============================================
  Files                 88       88           
  Lines               4077     4077           
  Branches             259      259           
==============================================
  Hits                3887     3887           
  Misses               138      138           
  Partials              52       52

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 109b522...daa9e2b. Read the comment docs.

stefanoborini · 2017-05-05T13:57:56Z

jstests/tests/home/test_configurables.js

    "home/configurables"
-], function (configurables) {
+], function ($, configurables) {


if you can workaround jquery, try. otherwise keep it.

Here I use jQuery for the extend method which does a deep copy of the dictionary representing the model of the configurable. Underscore provides an extendmethod too but it doesn't do a deep copy of the dictionary.
Instead of returning a list of dictionaries representing the models of configurables in configurable.jsI could return a list of classes. So we get rid of this deep copy of dictionaries.

stefanoborini · 2017-05-05T14:02:11Z

jstests/tests/home/test_configurables.js

-        assert.equal(resolution.as_config_dict().resolution, "1024x768");
-    });
-
-    QUnit.test("view", function (assert) {


in principle you can still use the view on a configurable component.

stefanoborini · 2017-05-05T14:06:57Z

remoteappmanager/static/js/home/configurables.js

+
+        data: function() {
+            return {
+                sel_value: this.value,


selected_value

stefanoborini · 2017-05-05T14:10:50Z

remoteappmanager/static/js/home/configurables.js

+        template:
+            '<div class="form-group">' +
+            '  <label>Resolution</label>' +
+            '  <select class="form-control" v-model="sel_value">' +


call it selected_value

stefanoborini · 2017-05-05T14:18:21Z

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

+            '            <fieldset v-else :disabled="current_app.is_starting()">' +
+            '              <component v-for="configurable in current_app.configurables"' +
+            '                         :is="configurable.tag"' +
+            '                         :value="configurable.value"' +


The .syncdoesn't work like expected... The child selected_value is not correctly synchronized with the parent value.

stefanoborini · 2017-05-05T14:26:57Z

remoteappmanager/static/js/home/configurables.js

-        '{{/each}}' +
-        '</div>'
-    );
+    var resolution_tag = 'resolution';


Vue recommends to use a hyphen.

maybe resolution-component?

make sure that you use reasonable differentiation between the configurable tag (e.g. "resolution") and the component tag (which is e.g. "resolution-component")

stefanoborini · 2017-05-05T14:35:03Z

remoteappmanager/static/js/home/models.js


-            if (ConfigurableCls !== null) {
-                app.configurables.push(new ConfigurableCls());
+            if (configurable !== null) {


I think it's undefined, not null, if you have an element that is not there.

stefanoborini · 2017-05-08T12:31:20Z

remoteappmanager/static/js/home/configurables.js

+    });
+
+    // Your configurable class must implement tag and value attributes and as_config_dict method
+    var resolution_model = function() {


Should be ResolutionModel (as a class)

which brings me to: do we really need it? I mean, technically, we could

have only the component.

have a component method as_config_dict that extracts the data from the component.

have the parent call this method when submit is called, aggregate all responses, and send them as part of the request

I totally agree with this, that's what I wanted to do first. But I faced some problems with this implementation:

How to call the as_config_dict method from the parent ? The parent here is ApplicationView and we won't have access to the child component and this method in the Javascript code, as far as I understand.

Where to define the value of resolution of each application if not in a model object ?

https://vuejs.org/v2/guide/components.html#Child-Component-Refs

each component uses a different data object. the parent does not need to know anything about the types of the children. It just needs to know that, once done, it will go through the list and invoke as_config_dict

Ok, great thank you :) So with this I can define as_config_dict directly in the component.
We still have the issue of defining a value associated to each application, do you have any idea ?

stefanoborini · 2017-05-08T12:38:11Z

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

+            '            <fieldset v-else :disabled="current_app.is_starting()">' +
+            '              <component v-for="configurable in current_app.configurables"' +
+            '                         :is="configurable.tag + \'-component\'"' +
+            '                         :value="configurable.value"' +


you can use .sync in this case. it should work, we are using vue 2.3

As I said in the other comment, I tried to use .sync but it didn't work. selected_value wasn't correctly synchronized with the parent's value. I'm not sure what was going on.

martinRenou · 2017-05-08T14:49:13Z

remoteappmanager/static/js/home/configurables.js

+    var ResolutionModel = function() {
+        this.tag = resolution_conf_tag;
+        this.value = 'Window';
+        this.component = resolution_component;


This line is only for the tests. It's really weird to have the component constructor in the model. I should change this.

By the way, I couldn't find a proper way to set the value for each application without creating a model.

stefanoborini · 2017-05-10T09:14:58Z

remoteappmanager/static/js/home/configurables.js

+    var resolution_conf_tag = 'resolution';
+    var resolution_component = Vue.component(resolution_conf_tag + '-component', {
+        // Your configurable must have a "value" property
+        props: ['value'],


config_dict ?

stefanoborini · 2017-05-10T09:16:15Z

remoteappmanager/static/js/home/configurables.js

+
+        data: function() {
+            return {
+                selected_value: this.value,


data that you accept is a bundle that contains specific information for the internals of the ui.
so in this case you have to extract data["resolution"] and put it here as the value for this specific widget of the UI.

stefanoborini · 2017-05-10T09:19:18Z

remoteappmanager/static/js/home/configurables.js

+    // Your configurable class must implement tag and value attributes
+    var ResolutionModel = function() {
+        this.tag = resolution_conf_tag;
+        this.value = 'Window';


this.data = {"resolution": "Window"}

stefanoborini · 2017-05-10T09:21:35Z

remoteappmanager/static/js/home/configurables.js

-            if (configurables[conf].prototype.tag === tag) {
-                return configurables[conf];
+
+                return { resolution: { resolution: resolution } };


just return for consistency with data that you put in.

stefanoborini · 2017-05-10T09:32:34Z

remoteappmanager/static/js/home/configurables.js

+        },
+
+        methods: {
+            as_config_dict: function() {


this should be in the model. All your interaction should be with the model, and the component is just a view that is inserted in the right place.

stefanoborini · 2017-05-10T09:40:36Z

remoteappmanager/static/js/home/models.js

@@ -121,15 +121,15 @@ define([
        // to its client-side model.
        app.configurables = [];

-        app.app_data.image.configurables.forEach(function(tag) {
+        app.app_data.image.configurables.forEach(function(conf_name) {


should be tag

he is here

martinRenou added 11 commits May 3, 2017 12:55

Remove trailing spaces

374e001

Start creating the component

a8eb69c

Component created

f629058

Try to insert component

abfe0ad

Merge branch 'switch_to_vue' into configurables_components

6f1e8bb

Try to not instantiate components

3565f2b

Just define a description of each configurable for each application.

First working version

2253541

Do not reinitialize configurables when updating the application

60bf323

Fix linter test

f07ebfb

Fix tests

fa76ab9

Disable configurable widgets when starting

3c60633

martinRenou changed the title ~~Resolution configurable is now a Vue component~~ Resolution configurable is now a Vue component [WIP] May 4, 2017

martinRenou added 4 commits May 4, 2017 14:33

Changed representation of the component

979eac3

And fixed the issue of config_dict not correctly set by default

Do a deep copy of the configurable model instead of getting each prop…

078e128

…erty

Add tests

aa4daae

Removed handlebars

bd8feee

martinRenou changed the title ~~Resolution configurable is now a Vue component [WIP]~~ Resolution configurable is now a Vue component May 4, 2017

martinRenou requested a review from stefanoborini May 4, 2017 14:18

stefanoborini suggested changes May 5, 2017

View reviewed changes

martinRenou added 3 commits May 5, 2017 16:31

Renamed sel_value to selected_value

84af7a0

Review

f6ba4b5

Seperate component tag name to configurable tag name

7dcc427

martinRenou changed the title ~~Resolution configurable is now a Vue component~~ Resolution configurable is now a Vue component [WIP] May 5, 2017

martinRenou added 2 commits May 8, 2017 09:58

Add tests for the resolution configurable component

cee2c2e

Configurable models are now classes

9aeeec8

martinRenou changed the title ~~Resolution configurable is now a Vue component [WIP]~~ Resolution configurable is now a Vue component May 8, 2017

stefanoborini suggested changes May 8, 2017

View reviewed changes

martinRenou added 2 commits May 8, 2017 13:54

Merge branch 'switch_to_vue' into configurables_components

7523ad9

Renamed resolution_model into ResolutionModel

b641e98

as_config_dict is now a component method

1d96bf9

martinRenou commented May 8, 2017

View reviewed changes

martinRenou added 5 commits May 8, 2017 16:21

Merge branch 'switch_to_vue' into configurables_components

8e51e49

Undo last change on the bower.json file

dc43241

Removed component from the ResolutionModel

79ee5f0

Remove useless test dom element

825bc1a

Add test

359b218

martinRenou changed the title ~~Resolution configurable is now a Vue component~~ Resolution configurable is now a Vue component [WIP] May 8, 2017

martinRenou changed the title ~~Resolution configurable is now a Vue component [WIP]~~ Resolution configurable is now a Vue component May 8, 2017

Merge branch 'switch_to_vue' into configurables_components

0eeec36

stefanoborini previously requested changes May 10, 2017

View reviewed changes

martinRenou added 4 commits May 10, 2017 11:12

Reviews

6b731d1

Fix tests

c12f7e5

Undo some changes in the model

26bc9f4

Use .sync modifier

daa9e2b

martinRenou added user part frontend labels May 11, 2017

martinRenou merged commit 6dcf0a0 into switch_to_vue May 11, 2017

martinRenou deleted the configurables_components branch May 11, 2017 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolution configurable is now a Vue component #420

Resolution configurable is now a Vue component #420

martinRenou commented May 4, 2017

codecov-io commented May 4, 2017 •

edited

Loading

stefanoborini May 5, 2017

martinRenou May 8, 2017

stefanoborini May 5, 2017

stefanoborini May 5, 2017

stefanoborini May 5, 2017

stefanoborini May 5, 2017

martinRenou May 5, 2017

stefanoborini May 5, 2017

stefanoborini May 5, 2017

stefanoborini May 5, 2017

stefanoborini May 5, 2017

stefanoborini May 8, 2017

stefanoborini May 8, 2017

martinRenou May 8, 2017 •

edited

Loading

stefanoborini May 8, 2017

martinRenou May 8, 2017

stefanoborini May 8, 2017

martinRenou May 8, 2017 •

edited

Loading

martinRenou May 8, 2017

martinRenou May 8, 2017

stefanoborini May 10, 2017

stefanoborini May 10, 2017

stefanoborini May 10, 2017

stefanoborini May 10, 2017

stefanoborini May 10, 2017

stefanoborini May 10, 2017

stefanoborini May 10, 2017

Resolution configurable is now a Vue component #420

Resolution configurable is now a Vue component #420

Conversation

martinRenou commented May 4, 2017

codecov-io commented May 4, 2017 • edited Loading

Codecov Report

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

martinRenou May 8, 2017 • edited Loading

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

martinRenou May 8, 2017 • edited Loading

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 May 4, 2017 •

edited

Loading

martinRenou May 8, 2017 •

edited

Loading

martinRenou May 8, 2017 •

edited

Loading