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

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

thanks for the review, pushing the updates.

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

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode381
app/app.js:381: // Null-queue for NS routing. The 1ms delay in the queue
presents
On 2013/02/14 17:58:27, matthew.scott wrote:
> Very minor, should be a /*...*/ style comment - will make it easy to
yuidoc down
> the road. Or it could just be yuidoc'd, if you want.

Something in me fights doing yuidoc for internal methods and overrides
that should never be called, but I can change it.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode395
app/app.js:395: var loc = Y.getLocation();
On 2013/02/14 17:58:27, matthew.scott wrote:
> Very minor: could be moved to top of function and used in line 391.

Done.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode404
app/app.js:404: /**
On 2013/02/14 17:58:27, matthew.scott wrote:
> We should note that this comes from App.Base, along with what has
changed.

Done.

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

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routing.js#newcode48
app/assets/javascripts/routing.js:48: * normalize a url w/o its qs.
On 2013/02/14 17:58:27, matthew.scott wrote:
> Normalize, without

Done.

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routing.js#newcode50
app/assets/javascripts/routing.js:50: * @return {Object} {url: string,
qs: querystring}.
On 2013/02/14 17:58:27, matthew.scott wrote:
> Should be returning just the URL, correct?

Done.

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

« Back to merge proposal