Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolution configurable is now a Vue component #420

Merged
merged 33 commits into from
May 11, 2017
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
374e001
Remove trailing spaces
martinRenou May 3, 2017
a8eb69c
Start creating the component
martinRenou May 3, 2017
f629058
Component created
martinRenou May 3, 2017
abfe0ad
Try to insert component
martinRenou May 3, 2017
6f1e8bb
Merge branch 'switch_to_vue' into configurables_components
martinRenou May 3, 2017
3565f2b
Try to not instantiate components
martinRenou May 4, 2017
2253541
First working version
martinRenou May 4, 2017
60bf323
Do not reinitialize configurables when updating the application
martinRenou May 4, 2017
f07ebfb
Fix linter test
martinRenou May 4, 2017
fa76ab9
Fix tests
martinRenou May 4, 2017
3c60633
Disable configurable widgets when starting
martinRenou May 4, 2017
979eac3
Changed representation of the component
martinRenou May 4, 2017
078e128
Do a deep copy of the configurable model instead of getting each prop…
martinRenou May 4, 2017
aa4daae
Add tests
martinRenou May 4, 2017
bd8feee
Removed handlebars
martinRenou May 4, 2017
84af7a0
Renamed sel_value to selected_value
martinRenou May 5, 2017
f6ba4b5
Review
martinRenou May 5, 2017
7dcc427
Seperate component tag name to configurable tag name
martinRenou May 5, 2017
cee2c2e
Add tests for the resolution configurable component
martinRenou May 8, 2017
9aeeec8
Configurable models are now classes
martinRenou May 8, 2017
7523ad9
Merge branch 'switch_to_vue' into configurables_components
martinRenou May 8, 2017
b641e98
Renamed resolution_model into ResolutionModel
martinRenou May 8, 2017
1d96bf9
as_config_dict is now a component method
martinRenou May 8, 2017
8e51e49
Merge branch 'switch_to_vue' into configurables_components
martinRenou May 8, 2017
dc43241
Undo last change on the bower.json file
martinRenou May 8, 2017
79ee5f0
Removed component from the ResolutionModel
martinRenou May 8, 2017
825bc1a
Remove useless test dom element
martinRenou May 8, 2017
359b218
Add test
martinRenou May 8, 2017
0eeec36
Merge branch 'switch_to_vue' into configurables_components
martinRenou May 10, 2017
6b731d1
Reviews
martinRenou May 10, 2017
c12f7e5
Fix tests
martinRenou May 10, 2017
26bc9f4
Undo some changes in the model
martinRenou May 10, 2017
daa9e2b
Use .sync modifier
martinRenou May 10, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion bower.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
"datatables.net": "~1.10.12",
"datatables.net-dt": "~1.10.12",
"admin-lte": "~2.3.8",
"handlebars": "~4.0.5",
"underscore": "~1.8.3",
"html5shiv": "~3.7.3",
"respond": "~1.4.2",
Expand Down
25 changes: 8 additions & 17 deletions jstests/tests/home/test_configurables.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,17 @@
define([
"jquery",
"home/configurables"
], function (configurables) {
], function ($, configurables) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"use strict";

QUnit.module("home.configurables");
QUnit.test("instantiation", function (assert) {
var resolution_class = configurables.from_tag("resolution");
var resolution = new resolution_class();
var resolution = $.extend(true, {}, configurables["resolution"]);

assert.equal(resolution.tag, "resolution");
assert.equal(resolution.resolution, "Window");

resolution.resolution = "1024x768";
assert.equal(resolution.as_config_dict().resolution, "1024x768");
});

QUnit.test("view", function (assert) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var resolution_class = configurables.from_tag("resolution");
var resolution = new resolution_class();
assert.equal(resolution.value, "Window");

var view = resolution.view();
assert.notEqual(view.find("select"), null);
assert.equal(view.find("option").length,
resolution.resolution_options.length);
resolution.value = "1024x768";
assert.equal(resolution.as_config_dict().resolution, "1024x768");
});
});
});
4 changes: 2 additions & 2 deletions jstests/tests/home/test_models.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ define([
"home/models"
], function (models) {
"use strict";

QUnit.module("home.models");
QUnit.test("instantiation", function (assert) {
var model = new models.ApplicationListModel();
assert.equal(model.app_list.length, 0);
model.update().done(function() {
assert.equal(model.app_list.length, 2);
assert.equal(model.app_list[0].app_data.image.configurables[0], "resolution");
assert.equal(model.app_list[0].configurables[0].resolution, "Window");
assert.equal(model.app_list[0].configurables[0].value, "Window");
});
});
});
Expand Down
3 changes: 1 addition & 2 deletions jstests/testsuite.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
bootstrap: '../components/bootstrap/js/bootstrap.min',
moment: "../components/moment/moment",
"jsapi/v1/resources": "../../../jstests/tests/home/mock_jsapi",
handlebars: "../components/handlebars/handlebars.amd.min",
underscore: "../components/underscore/underscore-min",
vue: "../components/vue/dist/vue"
},
Expand All @@ -25,7 +24,7 @@
"tests/home/test_views.js",
"tests/test_utils.js",
"tests/test_analytics.js"
], function(init) {
], function() {
window.apidata = {
base_url: "/",
prefix: "/"
Expand Down
120 changes: 44 additions & 76 deletions remoteappmanager/static/js/home/configurables.js
Original file line number Diff line number Diff line change
@@ -1,85 +1,53 @@
define([
"jquery",
"handlebars",
"utils"
], function($, hb, utils) {
"utils",
"components/vue/dist/vue.min"
], function(utils, Vue) {
"use strict";

var view_template = hb.compile(
'<div class="form-group">' +
'<label for="resolution-model-{{index}}">Resolution</label>' +
'<select class="form-control" id="resolution-model-{{ index }}">' +
'{{#each options}}' +
'<option value="{{this}}">{{this}}</option>' +
'{{/each}}' +
'</div>'
);
var resolution_tag = 'resolution';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vue recommends to use a hyphen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe resolution-component?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Vue.component(resolution_tag, {
// Your configurable must have a "value" property
props: ['value'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config_dict ?


var ResolutionModel = function () {
// Model for the resolution configurable.
var self = this;
this.resolution = "Window";
this.resolution_options = ["Window", "1920x1080", "1280x1024", "1280x800", "1024x768"];

self.view = function (index) {
// Creates the View to add to the application entry.
var widget = $(view_template({
options: self.resolution_options,
index: index
}));

widget.find("select").change(function() {
if (this.selectedIndex) {
self.resolution = this.options[this.selectedIndex].value;
}
});

return widget;
};
};

ResolutionModel.prototype.tag = "resolution";

ResolutionModel.prototype.as_config_dict = function() {
// Returns the configuration dict to hand over to the API request.
// e.g.
// {
// "resolution" : "1024x768"
// }
// The returned dictionary must be added to the configurable
// parameter under the key given by the tag member.
var resolution = this.resolution;

if (resolution === 'Window') {
var max_size = utils.max_iframe_size();
resolution = max_size[0]+"x"+max_size[1];
template:
'<div class="form-group">' +
' <label>Resolution</label>' +
' <select class="form-control" v-model="sel_value">' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call it selected_value

' <option v-for="resolution_option in resolution_options">' +
' {{resolution_option}}' +
' </option>' +
' </select>' +
'</div>',

data: function() {
return {
sel_value: this.value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selected_value

resolution_options: ['Window', '1920x1080', '1280x1024', '1280x800', '1024x768']
};
},

watch: {
value: function() {this.sel_value = this.value;}, // model -> view update
sel_value: function() {this.$emit('update:value', this.sel_value);} // view -> model update
}
return {
"resolution": resolution
};
};

// Define all your configurables here.
var configurables = {
ResolutionModel: ResolutionModel
};
var from_tag = function (tag) {
// Given a tag, lookup the appropriate configurable and
// return it. If the tag matches no configurable, returns null
for (var conf in configurables) {
if (configurables[conf].prototype.tag === tag) {
return configurables[conf];
});

// Export all your configurable models here
// (must implement tag and value attributes and as_config_dict method)
return {
resolution: {
tag: resolution_tag,
value: 'Window',
as_config_dict: function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var resolution = this.value;

if (this.value === 'Window') {
var max_size = utils.max_iframe_size();
resolution = max_size[0] + 'x' + max_size[1];
}

return { 'resolution': resolution };
}
}
return null;
};

var ns = {
from_tag: from_tag
};

return $.extend(ns, configurables);

});
12 changes: 5 additions & 7 deletions remoteappmanager/static/js/home/models.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ define([
.done(function(new_data) {
app.app_data = new_data;

this._update_configurables(app);
this._update_status(app);
}.bind(this));
};
Expand All @@ -121,15 +120,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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be tag

// If this returns null, the tag has not been recognized
// by the client. skip it and let the server deal with the
// missing data, either by using a default or throwing
// an error.
var ConfigurableCls = configurables.from_tag(tag);
var configurable = configurables[conf_name];

if (ConfigurableCls !== null) {
app.configurables.push(new ConfigurableCls());
if (configurable !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

app.configurables.push($.extend(true, {}, configurable));
}
});
};
Expand All @@ -151,8 +150,7 @@ define([

var configurables_data = {};
current_app.configurables.forEach(function(configurable) {
var tag = configurable.tag;
configurables_data[tag] = configurable.as_config_dict();
configurables_data[configurable.tag] = configurable.as_config_dict();
});

resources.Container.create({
Expand Down
10 changes: 9 additions & 1 deletion remoteappmanager/static/js/home/views/application_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,15 @@ define([
' </ul>' +

' <h4>Configuration</h4>' +
' <form class="configuration"><fieldset></fieldset></form>' +
' <form class="configuration">' +
' <fieldset v-if="current_app.configurables.length === 0">No configurable options for this image</fieldset>' +
' <fieldset v-else :disabled="current_app.is_starting()">' +
' <component v-for="configurable in current_app.configurables"' +
' :is="configurable.tag"' +
' :value="configurable.value"' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sync

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@martinRenou martinRenou May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

' @update:value="value => configurable.value = value"></component>' +
' </fieldset>' +
' </form>' +
' </div>' +

' <!-- Start Button -->' +
Expand Down