Code review comment for lp:~bcsaller/juju-gui/viewmodel-improvements

Revision history for this message
Nicola Larosa (teknico) wrote :

Land with changes.

This looks like a nice refactoring. Thanks for regenerating the
"undocumented" file. Please have a look at the comments below.

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#newcode102
app/views/topology/service.js:102: serviceDblClick: function(d, self) {
A more descriptive name than "d" on these methods would be nice, some
day.

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#newcode902
app/views/topology/service.js:902: destroyServiceConfirm: function(m,
view) {
Ditto for "m". Can we do away with these one-letter names, anyway? They
hamper readability.

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode611
app/views/utils.js:611: * Utility class that encapsulates Y.Models and
keeps their position
It is not a class anymore, is it? Can this comment be converted to a YUI
comment block somehow?

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode861
app/views/utils.js:861: * @param {ServiceModule} Module holding box
canvas and context.
s/Module/module/

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode865
app/views/utils.js:865: **/
Add a "@method toBoundingBoxes" directive, and remove one asterisk from
the block closing mark.

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode889
app/views/utils.js:889: * relation.getAttrs() plus "source", "target",
and other convenience data.
Indent the text, to show that it continues from the line above.

https://codereview.appspot.com/7231067/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7231067/diff/1/test/test_environment_view.js#newcode846
test/test_environment_view.js:846: // update using pos
s/pos/position/ ?

https://codereview.appspot.com/7231067/diff/1/test/test_environment_view.js#newcode887
test/test_environment_view.js:887:
boxes.wordpress.id.should.equal('wordpress');
Nicer. :-)

https://codereview.appspot.com/7231067/

« Back to merge proposal