Code review comment for lp:~rharding/juju-gui/browser_links

Revision history for this message
Jeff Pihach (hatch) wrote :

LGTM - There are a few rename and documentation requests below but good
work, thanks for working through this!

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode169
app/subapps/browser/browser.js:169: _stateChanged: function(field) {
These 4 methods which return true/false don't actually do what their
method names imply.

if (this._hasStateChanged()) {} would be easier to follow as you can
assume that there are no side effects.

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode305
app/subapps/browser/browser.js:305: Y.one('.bws-view-data').show();
Where does this element come from? Isn't it being rendered to the DOM
with the render() above?

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode355
app/subapps/browser/browser.js:355: renderSearchResults: function(req,
res, next) {
linter might complain about this not being documented with @method

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode379
app/subapps/browser/browser.js:379: if (this._showCharm()) {
See above comments - this looks like it's showing the charm and then
checking for a truthy return value that it's succeeded

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/views/editorial.js
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/views/editorial.js#newcode171
app/subapps/browser/views/editorial.js:171: renderTo: {
could we document these attrs please.

https://codereview.appspot.com/8726048/diff/15001/app/templates/charm-token.handlebars
File app/templates/charm-token.handlebars (right):

https://codereview.appspot.com/8726048/diff/15001/app/templates/charm-token.handlebars#newcode8
app/templates/charm-token.handlebars:8: <div class="charm-icon"></div>
I think the indentation is off here.

https://codereview.appspot.com/8726048/diff/15001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/8726048/diff/15001/test/test_browser_app.js#newcode333
test/test_browser_app.js:333: it('sidebar to details does no rerender
sidebar', function() {
what is this doing?

'renders details without re-rendering sidebar' ?

There are a few which aren't clear from their titles

https://codereview.appspot.com/8726048/

« Back to merge proposal