Popup Help Generated from new keyboard handling.

Popup Help Generated from new keyboard handling.

Replace exisitng keyboard support with new declarative system.
Generate popup help using declared bindings.

https://code.launchpad.net/~bcsaller/juju-gui/help-popup/+merge/162521

(do not edit description out of merge proposal)

[bcsaller, 2013-05-04 20:37:02]

Please take a look.

[matthew.scott, 2013-05-04 20:53:22]

LGTM.  I like this, it seems a lot easier to understand than what was in place
before.  I have a few style comments that the UI folks might override, so feel
free to ignore them if they do, but they just seem like they'd make the help
screen a little more readable.

Thanks!

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

https://codereview.appspot.com/9038045/diff/1/app/app.js#newcode156
app/app.js:156: // This could be its own view.
Perhaps worth a low bug/card?  Not a huuuge fan on template stuff in app.js, but
with a bug/card, I'd be more okay with it.

https://codereview.appspot.com/9038045/diff/1/app/app.js#newcode176
app/app.js:176: 'esc': {
Just a question, but would this be extensible in the future if we want esc to do
more, such as minimize the charm browser, clear state in env, etc?

https://codereview.appspot.com/9038045/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/9038045/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:290: .create-box-shadow(-2px 4px 4px 0 rgba(0, 0, 0,
0.5));
padding: 1em;

https://codereview.appspot.com/9038045/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:301: margin: 2em;
th {
  text-align: left;
}

and maybe text-align: center in .binding above.

[bcsaller, 2013-05-04 21:21:02]

Thanks for the review (and so quickly on a weekend)

Made your changes, still have to add the card.

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

https://codereview.appspot.com/9038045/diff/1/app/app.js#newcode156
app/app.js:156: // This could be its own view.
On 2013/05/04 20:53:22, matthew.scott wrote:
> Perhaps worth a low bug/card?  Not a huuuge fan on template stuff in app.js,
but
> with a bug/card, I'd be more okay with it.

I don't like it either but I think it still less code than was there before ;) I
should add a card.

https://codereview.appspot.com/9038045/diff/1/app/app.js#newcode176
app/app.js:176: 'esc': {
On 2013/05/04 20:53:22, matthew.scott wrote:
> Just a question, but would this be extensible in the future if we want esc to
do
> more, such as minimize the charm browser, clear state in env, etc?

We'd either add things to this list of cleanups or fire an event things can
listen on like your topo clearState (?) event

https://codereview.appspot.com/9038045/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/9038045/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:290: .create-box-shadow(-2px 4px 4px 0 rgba(0, 0, 0,
0.5));
On 2013/05/04 20:53:22, matthew.scott wrote:
> padding: 1em;

Done.

https://codereview.appspot.com/9038045/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:301: margin: 2em;
On 2013/05/04 20:53:22, matthew.scott wrote:
> th {
>   text-align: left;
> }
> 
> and maybe text-align: center in .binding above.

Done.

[bcsaller, 2013-05-04 21:22:28]

Please take a look.

[gary.poster, 2013-05-05 11:26:53]

LGTM. Thank you!

https://codereview.appspot.com/9038045/diff/5001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9038045/diff/5001/app/app.js#newcode187
app/app.js:187: // model, but I wanted to write it.
Yeah, I have wondered why that was the case, actually. Maybe something to change
in our reimp?

I do suggest abandoning for now, unfortunately. I wonder what we will want to do
about namespaces.

https://codereview.appspot.com/9038045/diff/5001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9038045/diff/5001/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
Maybe this should be default but let's discuss together.

https://codereview.appspot.com/9038045/diff/5001/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/9038045/diff/5001/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:281: #shortcut-help {
I want to QA this, and we should get a ux review, but I am ok with that as a
separate card so we can get this landed (as long as you follow up on the review
ASAP)

https://codereview.appspot.com/9038045/diff/5001/test/test_app_hotkeys.js
File test/test_app_hotkeys.js (right):

https://codereview.appspot.com/9038045/diff/5001/test/test_app_hotkeys.js#new...
test/test_app_hotkeys.js:36: container.remove(true);
Why doncha destroy the app too.

[bcsaller, 2013-05-05 17:17:35]

Thanks for the reviews, submitting.

https://codereview.appspot.com/9038045/diff/5001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9038045/diff/5001/app/app.js#newcode187
app/app.js:187: // model, but I wanted to write it.
On 2013/05/05 11:26:53, gary.poster wrote:
> Yeah, I have wondered why that was the case, actually. Maybe something to
change
> in our reimp?
> 
> I do suggest abandoning for now, unfortunately. I wonder what we will want to
do
> about namespaces. 

We could also register a function in the route definition such that 
units could return a route to their logical parent service and so on (though
thats about the only real case for this).

This would avoid the NS issue as well as the path manipulation isn't 
a function of the URL.

https://codereview.appspot.com/9038045/diff/5001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9038045/diff/5001/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
On 2013/05/05 11:26:53, gary.poster wrote:
> Maybe this should be default but let's discuss together. 

I just keep forgetting to turn it off after testing. Not trying to argue in
code. Though for devel it makes more sense than improv in todays world.

https://codereview.appspot.com/9038045/diff/5001/test/test_app_hotkeys.js
File test/test_app_hotkeys.js (right):

https://codereview.appspot.com/9038045/diff/5001/test/test_app_hotkeys.js#new...
test/test_app_hotkeys.js:36: container.remove(true);
On 2013/05/05 11:26:53, gary.poster wrote:
> Why doncha destroy the app too.

Done.

[bcsaller, 2013-05-05 17:29:14]

*** Submitted:

Popup Help Generated from new keyboard handling.

Replace exisitng keyboard support with new declarative system.
Generate popup help using declared bindings.

R=matthew.scott, gary.poster
CC=
https://codereview.appspot.com/9038045