Upon grabbing your branch for testing I see there are conflicts with trunk which include the issue of -right vs -up.
The branch looks good and behaved nicely.
Of course, wait for a Real Review.
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js File app/views/charm-search.js (right):
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode8 app/views/charm-search.js:8: subscriptions = [], A comment here describing the use and purpose of 'subscriptions' would be really helpful. I figured out but only when I got to the end of this file.
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode15 app/views/charm-search.js:15: icon = ev.currentTarget.one('i'); Is this line really repeated?
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode18 app/views/charm-search.js:18: icon.replaceClass('icon-chevron-right', 'icon-chevron-down'); -right should be -up to match the visual design. If you think the design is wrong then the charm panel section headers need to change too.
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode438 app/views/charm-search.js:438: container = Y.Node.create('<div></div>').setAttribute( Any tag created by Y.Node.create can be self-closing and it does the right thing. So
Y.Node.create('<div />')
works and is more readable, though you may argue misleading. I see we do both. Just mentioning it.
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode439 app/views/charm-search.js:439: 'id', 'juju-search-charm-panel'), Indent more
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode518 app/views/charm-search.js:518: headerSpan.setStyle('borderLeftColor', 'lightgray'); Why isn't this styling in CSS?
https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode534 app/views/charm-search.js:534: if (isPopupVisible) { These names with 'Popup' are historical and now inaccurate. Other places you refer to the panel (calculatePanelPosition). The use of both is confusing.
https://codereview.appspot.com/6775058/diff/18001/app/views/environment.js File app/views/environment.js (right):
https://codereview.appspot.com/6775058/diff/18001/app/views/environment.js#newcode1236 app/views/environment.js:1236: // "afterPageSizeRecalculation" event at the end of this function. Nice comment.
https://codereview.appspot.com/6775058/
« Back to merge proposal
Upon grabbing your branch for testing I see there are conflicts with
trunk which include the issue of -right vs -up.
The branch looks good and behaved nicely.
Of course, wait for a Real Review.
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js charm-search. js (right):
File app/views/
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode8 charm-search. js:8: subscriptions = [],
app/views/
A comment here describing the use and purpose of 'subscriptions' would
be really helpful. I figured out but only when I got to the end of this
file.
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode15 charm-search. js:15: icon = ev.currentTarge t.one(' i');
app/views/
Is this line really repeated?
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode18 charm-search. js:18: icon.replaceCla ss('icon- chevron- right', down');
app/views/
'icon-chevron-
-right should be -up to match the visual design. If you think the design
is wrong then the charm panel section headers need to change too.
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode438 charm-search. js:438: container = create( '<div>< /div>') .setAttribute(
app/views/
Y.Node.
Any tag created by Y.Node.create can be self-closing and it does the
right thing. So
Y.Node.create('<div />')
works and is more readable, though you may argue misleading. I see we
do both. Just mentioning it.
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode439 charm-search. js:439: 'id', 'juju-search- charm-panel' ),
app/views/
Indent more
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode518 charm-search. js:518: headerSpan. setStyle( 'borderLeftColo r',
app/views/
'lightgray');
Why isn't this styling in CSS?
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ charm-search. js#newcode534 charm-search. js:534: if (isPopupVisible) { Position) . The use of both
app/views/
These names with 'Popup' are historical and now inaccurate. Other
places you refer to the panel (calculatePanel
is confusing.
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ environment. js environment. js (right):
File app/views/
https:/ /codereview. appspot. com/6775058/ diff/18001/ app/views/ environment. js#newcode1236 environment. js:1236: // "afterPageSizeR ecalculation" event at
app/views/
the end of this function.
Nice comment.
https:/ /codereview. appspot. com/6775058/