Land with changes. Some very small suggestions below.
This test is not passing for me when I run the whole test suite. In isolation it is fine: http://pastebin.ubuntu.com/1507226/ I saw problems with zooming but you said it was pre-existing and something that you and Brad are working on. I'm OK with landing this if it's not making things worse. Thanks! Gary https://codereview.appspot.com/7071045/diff/1/app/app.js File app/app.js (right): https://codereview.appspot.com/7071045/diff/1/app/app.js#newcode754 app/app.js:754: // This alias doesn't seem to work, including refs by hand. Bug-worthy? https://codereview.appspot.com/7071045/diff/1/app/assets/javascripts/d3-components.js File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/7071045/diff/1/app/assets/javascripts/d3-components.js#newcode330 app/assets/javascripts/d3-components.js:330: * can trigger this after its sure relevant elements typo: it's https://codereview.appspot.com/7071045/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/7071045/diff/1/app/views/environment.js#newcode73 app/views/environment.js:73: // Bind d3 events (manually). Why do we have to do this here? Worth a comment? https://codereview.appspot.com/7071045/diff/1/app/views/topology/viewport.js File app/views/topology/viewport.js (right): https://codereview.appspot.com/7071045/diff/1/app/views/topology/viewport.js#newcode42 app/views/topology/viewport.js:42: // "afterPageSizeRecalculation" event at the end of this function. I suggest moving this comment to immediately before the pertinent code ("topo.fire('beforePageSizeRecalculation');") https://codereview.appspot.com/7071045/diff/1/package.json File package.json (right): https://codereview.appspot.com/7071045/diff/1/package.json#newcode20 package.json:20: "yeti": ">=0.2.0", we are not actually using this, are we? If so, I suggest we delete it. https://codereview.appspot.com/7071045/diff/1/test/index.html File test/index.html (right): https://codereview.appspot.com/7071045/diff/1/test/index.html#newcode31 test/index.html:31: <!-- Tests (Alphabetical)--> Cool, thanks :-) https://codereview.appspot.com/7071045/ -- https://code.launchpad.net/~bcsaller/juju-gui/viewport/+merge/142138 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/viewport 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
