yellow team mailing list archive

[Kapil Thangavelu <kapil.thangavelu@xxxxxxxxxxxxx>]

Looks pretty good. some minors below. could you a new card for the menu
scroll and positioning (ie. if too close to an edge, display on opposite
side).


https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#newcode1076
app/views/environment.js:1076: .ambiguousAddRelationTest(endpoint, self,
rect);
i'd suggest renaming this method, just to avoid the test parlance
outside of unit testing. maybe... addRelationEndpointCheck

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#newcode1476
app/views/environment.js:1476: // Add a cancel item.
The cancel item is better at the end, preferably styled as a dialog
cancel button or with red/differentiating text.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#newcode1477
app/views/environment.js:1477: menu.one('.cancel').on('click',
function(evt) {
The menu should also cancel on click outside, the same way the invalid
target fadeout works.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#newcode1483
app/views/environment.js:1483: var tr = view.zoom.translate(),
like the service menu this should also track roughly with the service
container on zoom/pan (the menu shouldn't resize though on zoom).

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#newcode1489
app/views/environment.js:1489: // Create a relation with the only
available endpoint.
i think this is better structured with the second clause upfront with a
return at the end of the block, and then the first block can become
dedented.

https://codereview.appspot.com/6736051/diff/3002/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/6736051/diff/3002/test/test_environment_view.js#newcode309
test/test_environment_view.js:309: add_rel =
container.one('#service-menu .add-relation'),
indent looks a little off here.

https://codereview.appspot.com/6736051/

-- 
https://code.launchpad.net/~makyo/juju-gui/ambiguous-relations/+merge/130616
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~makyo/juju-gui/ambiguous-relations into lp:juju-gui.