Thank you for the review Brad. I've made all requested changes. And intend to land this, per the new rules.
Gary 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 = [], On 2012/10/30 11:12:44, bac wrote: > 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. Done. https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode15 app/views/charm-search.js:15: icon = ev.currentTarget.one('i'); On 2012/10/30 11:12:44, bac wrote: > Is this line really repeated? Heh, yes. The duplication was in trunk (see lines 14 and 15 at left) but I did copy it blindly. Fixed, thank you. 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'); On 2012/10/30 11:12:44, bac wrote: > -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. I believe I've changed everything as desired. 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( On 2012/10/30 11:12:44, bac wrote: > 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. Yeah, I know. I felt mixed about the short form. I ended up using 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'), On 2012/10/30 11:12:44, bac wrote: > Indent more Done. https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode518 app/views/charm-search.js:518: headerSpan.setStyle('borderLeftColor', 'lightgray'); On 2012/10/30 11:12:44, bac wrote: > Why isn't this styling in CSS? We talked about the explanation, but I moved it to a class. https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode534 app/views/charm-search.js:534: if (isPopupVisible) { On 2012/10/30 11:12:44, bac wrote: > These names with 'Popup' are historical and now inaccurate. Other places you > refer to the panel (calculatePanelPosition). The use of both is confusing. Yeah...this comment led to a lot of changes :-) I corrected Panel -> Popup and I also corrected many instances of search -> panel, because that is also wrong. 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
