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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
