yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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.