yellow team mailing list archive

[Matthew Scott <matthew.scott@xxxxxxxxxxxxx>]

Thanks for the branch, Thiago.  I have a few minors, but one regression
that I'd like to see taken care of before I give a +1.


https://codereview.appspot.com/6849078/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode128
app/app.js:128: // F1...F12 or esc
Minor: This was unclear to me at first, and I had to stare at the next
line for a while to understand.  What you're testing for is any key that
is NOT a function key or esc without a modifier, correct?  I think that
this might be worth clarifying.

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode145
app/app.js:145: this.show_environment();
I'm not sure this is the right way to go about this, as it causes a
regression.  If I view a service, then hit alt+E, the environment view
is shown, but not navigated to, leaving the url, in my case,
http://localhost:8888/service/mysql/.  This breaks back-button behavior.

* view one service, mysql
* hit alt+E
* view another service, wordpress
* hit back: get taken to viewing mysql, because that was the last thing
in the URL stack.

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
Perhaps Y.Lang.isFunction(next), here?

https://codereview.appspot.com/6849078/

-- 
https://code.launchpad.net/~tveronezi/juju-gui/hotkeys/+merge/131382
Your team Juju GUI Hackers is subscribed to branch lp:juju-gui.