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

Revision history for this message
Madison Scott-Clary (makyo) wrote :

Some minors below. The code looks fine for the most part, with the
queue addition. However, I can't get the app to load past the "trying
to connect" screen. I'm seeing deltas in the websocket frames, but
nothing going on in the app. Will try uncommenting logs (so don't get
rid of them yet :) to see what's going on.

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#newcode346
app/app.js:346: //console.group('dispatch');
Remove commented code.

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
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.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode395
app/app.js:395: var loc = Y.getLocation();
Very minor: could be moved to top of function and used in line 391.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode404
app/app.js:404: /**
We should note that this comes from App.Base, along with what has
changed.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode996
app/app.js:996: * dispatches once per any dispatch call wrt namespace
with regards to

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.
Normalize, without

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}.
Should be returning just the URL, correct?

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

« Back to merge proposal