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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
