Code review comment for lp:~bcsaller/juju-gui/namespace-routing
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
I only got halfway through routing.js, but wanted to share what I had
before I went into calls. No one should wait for me: please don't count
me for the two reviews today. However, I will return to this if I can.
Gary
https:/
File app/assets/
https:/
app/assets/
copy and paste error: please update comment to describe what this
actually does (and the old comment has a typo, "particularly," but the
associated module has been removed from trunk).
https:/
app/assets/
trailing) {
I don't understand yet why this only trims a single character. My
suspicion is that, as I read, I'll understand that this is all you need,
and otherwise you would have been in the "oh my gosh JS doesn't have a
built-in regex excape function" place.
https:/
app/assets/
_trim(s, char, true, false); }
Wouldn't it be nice if the built-in trim were flexible enough for what
you need!
https:/
app/assets/
docstring would help with easy reading.
Return sorted array of object attribute pairs (name, value).
https:/
app/assets/
(TARDIS).
lol
https:/
app/assets/
trivial, but when placed next to your getQS implementation, "return
url.split('?')[0];" looks pretty reasonable.
https:/
app/assets/
url.split(
Might be helpful to include examples in comments:
> '/foo/bar'
["/foo/bar"]
'/> :baz:/foo/
["", ":baz:", "/foo/bar"]
https:/
app/assets/
namespace
Took me a second to agree, but yes, nice.
https:/
app/assets/
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.
https:/