yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

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.