Code review - Issue 6849078: application hotkeyshttps://codereview.appspot.com/2012-11-20T15:41:27+00:00rietveld
Message from unknown
2012-11-19T16:08:25+00:00thiagourn:md5:f8a88fb9734d0282002f169b3339e9d5
Message from thiago@veronezi.org
2012-11-19T16:08:30+00:00thiagourn:md5:040ed1bc379ef8e08c84c0e811c93366
Please take a look.
Message from matthew.scott@canonical.com
2012-11-19T18:36:51+00:00matthew.scotturn:md5:976329889406d553cdff518ce884af32
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?
Message from thiago@veronezi.org
2012-11-19T19:07:54+00:00thiagourn:md5:5a83887516077ef4a170e8aaef37e49a
Thanks for your review, Matt!
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
On 2012/11/19 18:36:51, matthew.scott wrote:
> 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.
Done.
https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode145
app/app.js:145: this.show_environment();
On 2012/11/19 18:36:51, matthew.scott wrote:
> 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.
Good catch. Thanks.
I've solved this issue by firing a 'navidateTo' event. So now instead of this.show_environment(), we call this.fire('navigateTo', { url: '/' });
https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
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?
Message from matthew.scott@canonical.com
2012-11-19T20:54:01+00:00matthew.scotturn:md5:7709cc8838a3b657c7c4313b65ec165b
Thanks for the reply! +1 with the navigateTo change
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#newcode145
app/app.js:145: this.show_environment();
On 2012/11/19 19:07:54, thiago wrote:
> On 2012/11/19 18:36:51, matthew.scott wrote:
> > 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.
>
> Good catch. Thanks.
> I've solved this issue by firing a 'navidateTo' event. So now instead of
> this.show_environment(), we call this.fire('navigateTo', { url: '/' });
Plugged this in, works well. Thanks!
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?
I don't feel that strongly either way :) Feel free to keep it as is, unless someone else minds.
Message from benjamin.saller@canonical.com
2012-11-20T13:26:32+00:00benjamin.sallerurn:md5:1cf89dfa066249ce1a5aaade61d8cbc3
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.
Message from thiago@veronezi.org
2012-11-20T14:53:59+00:00thiagourn:md5:84cec1d8580acf2e8ca17dec7e03d1c6
Thanks for your review Ben!
[]s,
Thiago.
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/20 13:26:32, benjamin.saller wrote:
> 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.
I fact, I dont need it now that I am calling the event instead of the show_environment method directly. I will remove it.
Message from unknown
2012-11-20T15:35:55+00:00thiagourn:md5:3fcc00878a0e87b9e41156e76789f7db
Message from unknown
2012-11-20T15:40:06+00:00thiagourn:md5:f3a6980f18fd08d66c99ca4796bb99ae
Message from thiago@veronezi.org
2012-11-20T15:41:27+00:00thiagourn:md5:3c47d5ddf811d53d994556862a938753
*** Submitted:
application hotkeys
The user can hit alt+e to open the environment view and alt+s to focus the
charm search field.
R=matthew.scott, benjamin.saller
CC=
https://codereview.appspot.com/6849078