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
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 browser/ browser. js (right):
File app/subapps/
https:/ /codereview. appspot. com/8726048/ diff/15001/ app/subapps/ browser/ browser. js#newcode169 browser/ browser. js:169: _stateChanged: function(field) {
app/subapps/
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 browser/ browser. js:305: Y.one(' .bws-view- data'). show();
app/subapps/
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 browser/ browser. js:355: renderSearchRes ults: function(req,
app/subapps/
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 browser/ browser. js:379: if (this._showCharm()) {
app/subapps/
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 browser/ views/editorial .js (right):
File app/subapps/
https:/ /codereview. appspot. com/8726048/ diff/15001/ app/subapps/ browser/ views/editorial .js#newcode171 browser/ views/editorial .js:171: renderTo: {
app/subapps/
could we document these attrs please.
https:/ /codereview. appspot. com/8726048/ diff/15001/ app/templates/ charm-token. handlebars charm-token. handlebars (right):
File app/templates/
https:/ /codereview. appspot. com/8726048/ diff/15001/ app/templates/ charm-token. handlebars# newcode8 charm-token. handlebars: 8: <div class=" charm-icon" ></div>
app/templates/
I think the indentation is off here.
https:/ /codereview. appspot. com/8726048/ diff/15001/ test/test_ browser_ app.js browser_ app.js (right):
File test/test_
https:/ /codereview. appspot. com/8726048/ diff/15001/ test/test_ browser_ app.js# newcode333 browser_ app.js: 333: it('sidebar to details does no rerender
test/test_
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/