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
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 topology/ service. js (right):
File app/views/
https:/ /codereview. appspot. com/7231067/ diff/1/ app/views/ topology/ service. js#newcode102 topology/ service. js:102: serviceDblClick: function(d, self) {
app/views/
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 topology/ service. js:902: destroyServiceC onfirm: function(m,
app/views/
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 utils.js: 611: * Utility class that encapsulates Y.Models and
app/views/
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 utils.js: 861: * @param {ServiceModule} Module holding box
app/views/
canvas and context.
s/Module/module/
https:/ /codereview. appspot. com/7231067/ diff/1/ app/views/ utils.js# newcode865 utils.js: 865: **/
app/views/
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 utils.js: 889: * relation.getAttrs() plus "source", "target",
app/views/
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 environment_ view.js (right):
File test/test_
https:/ /codereview. appspot. com/7231067/ diff/1/ test/test_ environment_ view.js# newcode846 environment_ view.js: 846: // update using pos
test/test_
s/pos/position/ ?
https:/ /codereview. appspot. com/7231067/ diff/1/ test/test_ environment_ view.js# newcode887 environment_ view.js: 887: .id.should. equal(' wordpress' );
test/test_
boxes.wordpress
Nicer. :-)
https:/ /codereview. appspot. com/7231067/