With the navigate change this looks fine. I might remove the //XXX: comment at the top of activate keys as even though we were not able to use the YUI framework for this it is working code and working code doesn't need to cry out for developer attention.
I'd also add a card indicating that we need design on a page bound to '?'|'h' or similar that can list the hotkeys (and prehaps a link to help in the base application template). These are useful but not discoverable. With that card in place, +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#newcode557 app/app.js:557: if (next) { On 2012/11/19 19:07:54, thiago wrote: > On 2012/11/19 18:36:51, matthew.scott wrote: > > Perhaps Y.Lang.isFunction(next), here? > Hmmm. Not shure. I just want to test if we have the next object (for unit tests > purposes). If the developer passes one thing other than a function, we should > just let the exception be thrown. wdyt? This is the 'next' route in the App.Router chain, the persistent views need this to continue processing, but they'd either all need an 'if (next) next();' guard or not. 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
