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/ -- https://code.launchpad.net/~gary/juju-gui/charmpanel/+merge/131605 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~gary/juju-gui/charmpanel into lp:juju-gui. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
