Fix navigateTo events to use nsRouter.

Fix navigateTo events to use nsRouter.

Fix the "navigateTo" event firings to define URLs by means of an
nsRouter instance.

Also change the "url" method in the
app/assets/javascripts/ns-routing-app-extension.js file, and related
tests, by changing the includeRootPaths flag into excludeRootPaths.
Including root paths in URLs becomes the default behavior, as agreed.

The code in app/views/charm.js was not changed: its status is not
clear, it does not seem current in its code structure and conventions,
and it's also not clear how to add an nsRouter object to its views.

https://code.launchpad.net/~teknico/juju-gui/1169511-use-nsrouter-in-navigateTo/+merge/159366

(do not edit description out of merge proposal)

[teknico, 2013-04-17 11:58:34]

Please take a look.

[gary.poster, 2013-04-17 12:19:27]

Hi Nicola.  LGTM with important changes given below, plus a test for your change
to combine, which would have caught the bug I identified.

(Reverting the char -> chr change is not important.  Now that you've done it, do
what you will in that regard.)

Gary

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

https://codereview.appspot.com/8826043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:5: function _trim(s, char,
leading, trailing) {
char -> chr seems a bit gratuitous to me.  I think char is clearer, and also has
the weight of being already written.

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

https://codereview.appspot.com/8826043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:207: true); //
excludeRootPaths
This is not correct, which makes me worry about the tests.  This should be
{excludeRootPaths: true}.

[teknico, 2013-04-17 14:28:17]

Thanks for the review, Gary.

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

https://codereview.appspot.com/8826043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:5: function _trim(s, char,
leading, trailing) {
gary.poster wrote:
> char -> chr seems a bit gratuitous to me.  I think char is clearer,
> and also has the weight of being already written.

Oh Gosh. Sorry about that, reverting. I forgot to motivate it.

What happened is that Sublime-SublimeLinter-jshint said "reserved word!", but
there's no mention of it in ECMA-262. I now tried to understand what the heck,
but ERABBITHOLETOODEEP, some other time.

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

https://codereview.appspot.com/8826043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:207: true); //
excludeRootPaths
gary.poster wrote:
> This is not correct, which makes me worry about the tests.
> This should be {excludeRootPaths: true}.

Indeed. I added a test that does fail, and then fixed it. Thanks.

[teknico, 2013-04-17 14:37:27]

Please take a look.

[jeff.pihach, 2013-04-17 15:07:24]

LGTM with a trvial suggestion, do with it what you wish :)

https://codereview.appspot.com/8826043/diff/6001/app/assets/javascripts/ns-ro...
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8826043/diff/6001/app/assets/javascripts/ns-ro...
app/assets/javascripts/ns-routing-app-extension.js:171: if (!(base[ns] === '/'
&& options.excludeRootPaths)) {
I'm not a fan of the negating checks as it makes it harder to read.

if ((base[ns] !== '/') && (!options.excludeRootPaths))

[frankban, 2013-04-17 15:09:36]

LGTM, thanks!

[teknico, 2013-04-17 15:13:17]

jeff.pihach wrote:
> I'm not a fan of the negating checks as it makes it harder
> to read.
> 
> if ((base[ns] !== '/') && (!options.excludeRootPaths))

Ok, so you take out one negation, and add back in two :-) And by the way, I
think it's:

((base[ns] !== '/') || (!options.excludeRootPaths))

I'm not entirely sure which one is more readable. I did consider the
alternative, but its meaning seems less clear to me.

[teknico, 2013-04-17 15:22:58]

*** Submitted:

Fix navigateTo events to use nsRouter.

Fix the "navigateTo" event firings to define URLs by means of an
nsRouter instance.

Also change the "url" method in the
app/assets/javascripts/ns-routing-app-extension.js file, and related
tests, by changing the includeRootPaths flag into excludeRootPaths.
Including root paths in URLs becomes the default behavior, as agreed.

The code in app/views/charm.js was not changed: its status is not
clear, it does not seem current in its code structure and conventions,
and it's also not clear how to add an nsRouter object to its views.

R=gary.poster, jeff.pihach, frankban
CC=
https://codereview.appspot.com/8826043