Code review comment for lp:~bcsaller/juju-gui/namespace-routing

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Made changes around your suggestions, but will wait for other reviews

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routing.js
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routing.js#newcode37
app/assets/javascripts/routing.js:37: // Multi dimensional router
(TARDIS).
On 2013/02/14 14:00:09, gary.poster wrote:
> lol

I forgot I left that in there. :)

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routing.js#newcode49
app/assets/javascripts/routing.js:49: return parts[0];
On 2013/02/14 14:00:09, gary.poster wrote:
> trivial, but when placed next to your getQS implementation, "return
> url.split('?')[0];" looks pretty reasonable.

Done.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routing.js#newcode65
app/assets/javascripts/routing.js:65: var parts =
url.split(this.fragment);
On 2013/02/14 14:00:09, gary.poster wrote:
> Might be helpful to include examples in comments:

> > '/foo/bar'.split(/\/?(:\w+\:)/)
> ["/foo/bar"]
> '/> :baz:/foo/bar'.split(/\/?(:\w+\:)/)
> ["", ":baz:", "/foo/bar"]

Done.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routing.js#newcode68
app/assets/javascripts/routing.js:68: result.charmstore = parts[0];
On 2013/02/14 14:00:09, gary.poster wrote:
> wait a second, that's not general-purpose! :-)

> I don't see the need for this to be general purpose. I suggest moving
it into
> app/routing.js. Alternatively, if you really want to, clean this up.

Added this.defaultNamespace and use that in place.

https://codereview.appspot.com/7327045/

« Back to merge proposal