Moved the ns routing code into an extension

Moved the ns routing code into an extension



https://code.launchpad.net/~hatch/juju-gui/ns-routing-extension/+merge/153240

(do not edit description out of merge proposal)

[jeff.pihach, 2013-03-13 20:43:19]

Please take a look.

[gary.poster, 2013-03-13 21:14:08]

LGTM!  Very nice.  I'd like to see some trivial comment cleanups that I mention,
but that's it.  Thanks!

Gary

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

https://codereview.appspot.com/7757046/diff/1/app/app.js#newcode31
app/app.js:31: Y.juju.SubAppRegistration,
Wow, I don't think that beautifier and I will ever see eye-to-eye...

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:5: // Below copied directly
from routing.js without modification
Is this comment maybe out of date?  It looks like you combined routing.js into
this one, in which case you can just delete this comment, and the one below it.

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:211: // End routing.js copy
This comment, I mean.

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:221: // working with
namespaced URLs.  See the module for details.
"See the module for details"?  We are looking at the module, aren't we?  Or is
this displayed by yuidoc?

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:222: this._nsRouter =
Y.juju.Router(this.defaultNamespace);
I think I need to ask you about this.  I thought we couldn't use Y.juju because
of how we were doing our namespaces, but obviously we can...

[matthew.scott, 2013-03-13 22:07:55]

LGTM, with Gary's comments accounted for.

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

https://codereview.appspot.com/7757046/diff/1/app/app.js#newcode251
app/app.js:251: // These two attributes are used by the namespaced URL tracker.
'two attributes' maybe no longer applies?

https://codereview.appspot.com/7757046/diff/1/test/test_routing.js
File test/test_routing.js (right):

https://codereview.appspot.com/7757046/diff/1/test/test_routing.js#newcode18
test/test_routing.js:18: 
Super minor, but probably not necessary.

[jeff.pihach, 2013-03-13 22:37:06]

Thanks for the reviews guys! Comments fixed

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

https://codereview.appspot.com/7757046/diff/1/app/app.js#newcode251
app/app.js:251: // These two attributes are used by the namespaced URL tracker.
On 2013/03/13 22:07:55, matthew.scott wrote:
> 'two attributes' maybe no longer applies?
Good catch!

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:5: // Below copied directly
from routing.js without modification
On 2013/03/13 21:14:08, gary.poster wrote:
> Is this comment maybe out of date?  It looks like you combined routing.js into
> this one, in which case you can just delete this comment, and the one below
it.
Good idea

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:211: // End routing.js copy
On 2013/03/13 21:14:08, gary.poster wrote:
> This comment, I mean.
gone!

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:221: // working with
namespaced URLs.  See the module for details.
On 2013/03/13 21:14:08, gary.poster wrote:
> "See the module for details"?  We are looking at the module, aren't we?  Or is
> this displayed by yuidoc?
Yes we are now in the module :) copy/paste error - fixed.

https://codereview.appspot.com/7757046/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:222: this._nsRouter =
Y.juju.Router(this.defaultNamespace);
On 2013/03/13 21:14:08, gary.poster wrote:
> I think I need to ask you about this.  I thought we couldn't use Y.juju
because
> of how we were doing our namespaces, but obviously we can...
Changed to use namspace() for consistency as we discussed.

https://codereview.appspot.com/7757046/diff/1/test/test_routing.js
File test/test_routing.js (right):

https://codereview.appspot.com/7757046/diff/1/test/test_routing.js#newcode18
test/test_routing.js:18: 
On 2013/03/13 22:07:55, matthew.scott wrote:
> Super minor, but probably not necessary.

space...nuked!

[jeff.pihach, 2013-03-13 22:48:23]

*** Submitted:

Moved the ns routing code into an extension

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