juju-gui

Code review comment for lp://staging/~gary/juju-gui/sandboxEnv-4

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-03-21:

Thanks for this! Mostly trivial comments except for the one in
test_app.js which we are chatting about on IRC.

https://codereview.appspot.com/7896045/diff/1/app/index.html#newcode237
app/index.html:237: setLoadingMessageText('Connecting to the Juju
environment');
Yay! Thinking positive! :-)

https://codereview.appspot.com/7896045/diff/1/app/store/env/sandbox.js#newcode244
app/store/env/sandbox.js:244: // For fuller verisimilitude, we could
convert some of the
I think that this comment needs a comment defining what `verisimilitude`
means

;-)

https://codereview.appspot.com/7896045/diff/1/app/store/env/sandbox.js#newcode248
app/store/env/sandbox.js:248: // The unit change_type is actually
"serviceUnit" in the Python
possible typo? change_type to be changeType

https://codereview.appspot.com/7896045/diff/1/app/store/env/sandbox.js#newcode327
app/store/env/sandbox.js:327: performOp_deploy: function(data) {
Instead of using _ on these methods to match the previous fn maybe we
should change the old one to camelCase

https://codereview.appspot.com/7896045/diff/1/test/test_app.js#newcode408
test/test_app.js:408: app = new Y.juju.App(
The app now requires that an element is in the DOM at the time of
instantiation (that's in the index.html by default) so this test will
fail once being merged with trunk.