yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

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?

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.