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"]
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
Made changes around your suggestions, but will wait for other reviews
https:/ /codereview. appspot. com/7327045/ diff/3001/ app/assets/ javascripts/ routing. js javascripts/ routing. js (right):
File app/assets/
https:/ /codereview. appspot. com/7327045/ diff/3001/ app/assets/ javascripts/ routing. js#newcode37 javascripts/ routing. js:37: // Multi dimensional router
app/assets/
(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 javascripts/ routing. js:49: return parts[0];
app/assets/
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 javascripts/ routing. js:65: var parts = this.fragment) ;
app/assets/
url.split(
On 2013/02/14 14:00:09, gary.poster wrote:
> Might be helpful to include examples in comments:
> > '/foo/bar' .split( /\/?(:\ w+\:)/) bar'.split( /\/?(:\ w+\:)/)
> ["/foo/bar"]
> '/> :baz:/foo/
> ["", ":baz:", "/foo/bar"]
Done.
https:/ /codereview. appspot. com/7327045/ diff/3001/ app/assets/ javascripts/ routing. js#newcode68 javascripts/ routing. js:68: result.charmstore = parts[0];
app/assets/
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.defaultNam espace and use that in place.
https:/ /codereview. appspot. com/7327045/