Code review comment for lp://staging/~gary/juju-gui/bug1167967

On 2013/04/11 20:10:37, bcsaller wrote:
> LGTM.

> Some suggestions around the url handling but take it or leave it.

Thank you for the thoughtful review.

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

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode171
> app/assets/javascripts/ns-routing-app-extension.js:171: url += ':' +
ns + ':' +
> base[ns];
> Maybe a flag to the method to control this behavior so we can use this
method to
> generate all such links?

I agonized about this comment a bit. First I made the new behavior
optional, and wrote tests. Then I decided that I thought the new
behavior ought to be the easy, new one, because I think it is what we
usually want, and undid all the option work. Then I decided to make an
option to have the old behavior...but I still kept the non-calculated
root strings ('/:gui:/') because I thought they were clear enough and
short enough. I don't know. I'm still a bit conflicted, but I feel
like removing root paths is the right thing to do, and the right thing
to make easy.

> https://codereview.appspot.com/8680043/diff/1/app/views/charm.js
> File app/views/charm.js (right):

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
> app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
> In cases like this I previously recommended that we use nsRouter.url
to create
> the URL. That no longer works in this case as '/' becomes implicit in
the
> namespace.

For this case, right. I could use the flag I added, but I currently
think that the string is fine for a root path.

> I like the abstraction of something that can build and assemble proper
URLs for
> the developer. Maybe a flag to nsRouter.url that forces inclusion.
I'll make a
> note in the router code as well but its up to you if you agree or not.

Yeah, I agree but it conflicts with other desires I have for this.

> https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js
> File app/views/landscape.js (left):

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js#oldcode132
> app/views/landscape.js:132: if (!modelAnnotation) {
> oops. While improv doesn't add these annotations to new nodes it was
my
> understanding that landscape will? Still, I'm sure there is going to
be some
> latency in that process and these checks are valid.

Yeah, that's what I thought.

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

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js#newcode53
> test/test_routing.js:53: assert.strictEqual(u, '/:foo:/shazam/');
> The reason I originally went the other way was I though presence in
the URL
> could indicate state, such that :inspector:/ might indicate an open
panel. I
> think you're right though that there are other ways to model it and
making '/'
> implicit is fine.

Cool, yeah.

https://codereview.appspot.com/8680043/