I make several suggestions but they are all straight-forward so no re-look is necessary before landing.
Recently I *think* we agreed to the 'one var per variable' idea which you didn't follow for the newly added code. Perhaps it was "When in Rome" or you just forgot. I'm just mentioning as a point of discussion not a suggestion to change as it would delay this important work from landing. https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-components.js File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-components.js#newcode155 app/assets/javascripts/d3-components.js:155: These two cases are mutually exclusive, right, so 'else if' would be clearer. https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-components.js#newcode248 app/assets/javascripts/d3-components.js:248: if (Y.Array.indexOf(['windowresize'], name) !== -1) { I don't understand why the indexOf is used instead of just a comparison. https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-components.js#newcode315 app/assets/javascripts/d3-components.js:315: // Create an adaptor s/adaptor/adapter https://codereview.appspot.com/6971045/diff/17/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6971045/diff/17/app/views/environment.js#newcode66 app/views/environment.js:66: // Bind d3 events (manually) This line is a complete thought and needs a period to be more readable. https://codereview.appspot.com/6971045/diff/17/app/views/environment.js#newcode69 app/views/environment.js:69: // the existing (from change to showView) I'm unclear what you're saying here. https://codereview.appspot.com/6971045/diff/17/app/views/topology/panzoom.js File app/views/topology/panzoom.js (right): https://codereview.appspot.com/6971045/diff/17/app/views/topology/panzoom.js#newcode9 app/views/topology/panzoom.js:9: * Handle PanZoom within the a Topology. typo: the a Pick one? :) https://codereview.appspot.com/6971045/diff/17/app/views/topology/panzoom.js#newcode51 app/views/topology/panzoom.js:51: contianer = topo.get('container'), typo: container Is that variable even used? https://codereview.appspot.com/6971045/diff/17/app/views/topology/panzoom.js#newcode127 app/views/topology/panzoom.js:127: evt.translate[1] -= parseInt(rect.attr('height'), 10) / 2 * delta; I wish we'd made a wrapper early on for parseInt that defaulted to base 10. So annoying to see everywhere. https://codereview.appspot.com/6971045/diff/17/app/views/topology/topology.js File app/views/topology/topology.js (right): https://codereview.appspot.com/6971045/diff/17/app/views/topology/topology.js#newcode17 app/views/topology/topology.js:17: * Emmitted Events: Emitted https://codereview.appspot.com/6971045/ -- https://code.launchpad.net/~bcsaller/juju-gui/topology-panzoom/+merge/140671 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/topology-panzoom 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
