Skip to content

Replace application view with a Vue object#405

Merged
martinRenou merged 24 commits into
switch_to_vuefrom
replace_application_view
Apr 26, 2017
Merged

Replace application view with a Vue object#405
martinRenou merged 24 commits into
switch_to_vuefrom
replace_application_view

Conversation

@martinRenou

Copy link
Copy Markdown
Member

No description provided.

@codecov-io

codecov-io commented Apr 26, 2017

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@              Coverage Diff               @@
##           switch_to_vue     #405   +/-   ##
==============================================
  Coverage          95.15%   95.15%           
==============================================
  Files                 94       94           
  Lines               4047     4047           
  Branches             254      254           
==============================================
  Hits                3851     3851           
  Misses               143      143           
  Partials              53       53

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 ef3b360...31ab258. Read the comment docs.

Comment thread jstests/testsuite.js
});

require([
"home/init",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Remove handlebars from bower

application_view) {
"use strict";

var ga = gamodule.init();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you can extract a separate view specifically for ga events, and respond to model changes by sending ga events.

dialogs.webapi_error_dialog(error);
});
};
var app_list_view = new application_list_view.ApplicationListView();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

try to see why it's not working.

var app_view = new application_view.ApplicationView();

$.when(model.update()).done(function () {
app_list_view.loading = false;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

move the loading to the model. loading is part of the state of the model.

app_list_view.model = model;

app_view.render(false, 200);
app_list_view.model = model;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

move this outside and remove the done() reaction. Just call model.update()

return image.ui_name? image.ui_name: image.name;
}
}
filters: utils.filters

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you can register filters globally.

@@ -1,170 +1,91 @@
define([
"jquery",
'jquery',

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

quotes


data: function() {
return {
model: { app_list: [], selected_index: null }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

probably, like the other one, is irrelevant, because you pass the model from outside.

return output;
},
iframe_size: function() {
return utils.max_iframe_size();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is not a dependent property so it gets cached forever.
You need a method.

eventLabel: current_app.app_data.image.name
});

this.model.update_idx(selected_index)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not now, but the model should really do this operation.

@martinRenou martinRenou merged commit 062c8c7 into switch_to_vue Apr 26, 2017
@martinRenou martinRenou deleted the replace_application_view branch April 26, 2017 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants