Panzoom Module

Panzoom Module This branch breaks out the first module from Mega into a more module unit. While this module is small a number of changes occur to make this happen. The framework underwent some improvemnts, changes around interaction with App and view replacement occured, event bindings had to be updated. A pattern for cross module event firing was established. In future modules, the topo/component fires the events and modules are bubble targets. https://code.launchpad.net/~bcsaller/juju-gui/topology-panzoom/+merge/140671 (do not edit description out of merge proposal)

[bcsaller, 2012-12-19 13:50:18]

Please take a look.

[bcsaller, 2012-12-20 13:39:21]

Please take a look.

[bcsaller, 2012-12-20 14:49:03]

Please take a look.

[gary.poster, 2012-12-20 15:23:26]

Thanks for helping me understand this. As I expected, I'm +1 on the code. I would like a bug/card to fix the prod tests on start. I'd like to see if we can fix the viewport resize issue one way or another before we land. Thank you Gary https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-comp... File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-comp... app/assets/javascripts/d3-components.js:328: bindAllD3Events: function() { Comment describing behavior and saying that the expected client of this is the component might be nice. 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#... app/views/topology/panzoom.js:107: * Wraper around the actual rescale method for zoom buttons. Wrapper

[hazmat, 2012-12-20 15:43:29]

quick items https://codereview.appspot.com/6971045/diff/7002/app/assets/javascripts/d3-co... File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6971045/diff/7002/app/assets/javascripts/d3-co... app/assets/javascripts/d3-components.js:255: console.log('Bind', target, eventPhase, name); s/console.log // console.debug .. s/'Bind'/'d3 components bind'

[bcsaller, 2012-12-20 17:23:55]

*** Submitted: Panzoom Module This branch breaks out the first module from Mega into a more module unit. While this module is small a number of changes occur to make this happen. The framework underwent some improvemnts, changes around interaction with App and view replacement occured, event bindings had to be updated. A pattern for cross module event firing was established. In future modules, the topo/component fires the events and modules are bubble targets. R=gary.poster, hazmat CC= https://codereview.appspot.com/6971045 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#newco... app/views/environment.js:27: broadcast: true, Test again w/o broadcast. https://codereview.appspot.com/6971045/diff/17/app/views/topology/mega.js File app/views/topology/mega.js (right): https://codereview.appspot.com/6971045/diff/17/app/views/topology/mega.js#new... app/views/topology/mega.js:872: self.cursorBox = new views.BoundingBox(); need new here?

[bac, 2012-12-20 17:51:40]

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-comp... File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6971045/diff/17/app/assets/javascripts/d3-comp... 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-comp... 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-comp... 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#newco... 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#newco... 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#... 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#... 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#... 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... app/views/topology/topology.js:17: * Emmitted Events: Emitted