Fixed navigate event bubbling issue

Fixed navigate event bubbling issue



https://code.launchpad.net/~hatch/juju-gui/1137993-bubble-target/+merge/151321

(do not edit description out of merge proposal)

[jeff.pihach, 2013-03-01 22:13:25]

Please take a look.

[gary.poster, 2013-03-02 03:15:06]

LGTM with trivial.  Thanks!

Gary

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/app-suba...
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/app-suba...
app/assets/javascripts/app-subapp-extension.js:45: this.navigate(':' +
e.namespace + ':' + e.url);
cool.  This would have taken me a long time to figure out. :-)

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:48: namespace: this.get('urlNamespace')
perhaps this should be something you can override?  This is a default?  YAGNI,
maybe.

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:66: avoid throwing errors on history change
events
Please add comment about how we can remove this once we move to YUI 3.1 or
whatever it is. :-)

https://codereview.appspot.com/7448047/diff/1/package.json
File package.json (right):

https://codereview.appspot.com/7448047/diff/1/package.json#newcode35
package.json:35: "node-spritesheet": "0.4.0",
I'm assuming this is a fly-by to fix another backwards incompatibility in one of
our dependencies.  Fine.  I'd be fine with specifying exact versions for all of
our dependencies, but that has its downsides.

[jeff.pihach, 2013-03-04 13:50:43]

Thanks for the review! Comments below.

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/app-suba...
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/app-suba...
app/assets/javascripts/app-subapp-extension.js:45: this.navigate(':' +
e.namespace + ':' + e.url);
On 2013/03/02 03:15:06, gary.poster wrote:
> cool.  This would have taken me a long time to figure out. :-)
I'll add some inline comments so that I know what's going on here in a months
time :^)

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:48: namespace: this.get('urlNamespace')
On 2013/03/02 03:15:06, gary.poster wrote:
> perhaps this should be something you can override?  This is a default?  YAGNI,
> maybe.
This property is referenced by the resulting navigate call on the parent so that
it knows which namespace to update in the url (and then to parse).

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:66: avoid throwing errors on history change
events
On 2013/03/02 03:15:06, gary.poster wrote:
> Please add comment about how we can remove this once we move to YUI 3.1 or
> whatever it is. :-)
You got it!

https://codereview.appspot.com/7448047/diff/1/package.json
File package.json (right):

https://codereview.appspot.com/7448047/diff/1/package.json#newcode35
package.json:35: "node-spritesheet": "0.4.0",
On 2013/03/02 03:15:06, gary.poster wrote:
> I'm assuming this is a fly-by to fix another backwards incompatibility in one
of
> our dependencies.  Fine.  I'd be fine with specifying exact versions for all
of
> our dependencies, but that has its downsides.
Yeah I am not sure what broke with this but it appears to be something with
ImageMagik - Will have to investigate but at the time I just wanted to get the
work done.

[bcsaller, 2013-03-04 14:03:09]

LGTM

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/app-suba...
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/app-suba...
app/assets/javascripts/app-subapp-extension.js:45: this.navigate(':' +
e.namespace + ':' + e.url);
On 2013/03/04 13:50:43, jeff.pihach wrote:
> On 2013/03/02 03:15:06, gary.poster wrote:
> > cool.  This would have taken me a long time to figure out. :-)
> I'll add some inline comments so that I know what's going on here in a months
> time :^)

In the future I would like to take namespace from the additional event arguments
so we don't need to construct strings like this. 

In the meantime its might be better to use the API _nsRouter.combine({namespace:
url}) but up to you.

[benji.york, 2013-03-04 14:05:25]

LGTM with trivial

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7448047/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:69: @protected
We need a @return

[jeff.pihach, 2013-03-04 15:19:02]

*** Submitted:

Fixed navigate event bubbling issue

R=gary.poster, bcsaller, benji.york
CC=
https://codereview.appspot.com/7448047