Thanks for the review Benji. I think I've addressed all of your concerns. After I respond to Francesco's concerns I'll push a new proposal.
https://codereview.appspot.com/6842119/diff/1/app/store/charm.js File app/store/charm.js (right): https://codereview.appspot.com/6842119/diff/1/app/store/charm.js#newcode74 app/store/charm.js:74: is_subordinate: result.subordinate}); is_subordinate is a pre-existing property on a charm. Yeah, we may need an effort to bring them all in-line but I'd rather not change existing stuff now. https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-result.handlebars File app/templates/charm-search-result.handlebars (right): https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-result.handlebars#newcode6 app/templates/charm-search-result.handlebars:6: <li class="picker-item all" data-filter="all">All charms ({{all_charms_count}})</li> On 2012/11/30 20:37:47, benji wrote: > The use of "data" attributes is nice. I learned it from you! :) https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-result.handlebars#newcode9 app/templates/charm-search-result.handlebars:9: </ul> On 2012/11/30 20:37:47, benji wrote: > There are some long lines here. Fixed except as noted below. https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-result.handlebars#newcode33 app/templates/charm-search-result.handlebars:33: href="{{id}}">{{#if owner}}{{owner}}/{{/if}}{{package_name}}</a> This line is 79, it needs to remain due to the comment above. I could have the view export a variable to get around the logic here but that seems extreme to just avoid a longish line. https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js File app/views/charm-panel.js (right): https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode85 app/views/charm-panel.js:85: var deployed_charms, On 2012/11/30 20:37:47, benji wrote: > We have recently switched to one-var-per-variable. Done. https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode93 app/views/charm-panel.js:93: is_sub_filter = function(charm) { On 2012/11/30 20:37:47, benji wrote: > I think this file uses enough camel case that these new variables should be > camel case too. Done. https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode134 app/views/charm-panel.js:134: series_group.charms = sub_charms; On 2012/11/30 20:37:47, benji wrote: > If you wanted, these two lines could be combined and the "sub_charms" variable > removed. Done. https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode257 app/views/charm-panel.js:257: //this.render(); On 2012/11/30 20:37:47, benji wrote: > Commented code accidentally left in. Entire handler is removed. https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode276 app/views/charm-panel.js:276: if (true) { // Avoid lint warning. On 2012/11/30 20:37:47, benji wrote: > Ignorable punctiliousness: that's a whole lot of spaces before that comment. Done. https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less File lib/views/stylesheet.less (right): https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less#newcode692 lib/views/stylesheet.less:692: /* XXX bac: The positioning is off but I have been unable On 2012/11/30 20:37:47, benji wrote: > Long lines. Done. https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less#newcode760 lib/views/stylesheet.less:760: /*#zoom-btn-up { On 2012/11/30 20:37:47, benji wrote: > Commented-out code. This code is not new, just refactored, so I left in the commented code. I've removed it now. https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js File test/test_charm_panel.js (right): https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcode324 test/test_charm_panel.js:324: Y.one('#main').append(Y.Node.create( On 2012/11/30 20:37:47, benji wrote: > We have transitioned to using the chained node methods to create DOM nodes > instead of strings with HTML in them. Done. https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcode328 test/test_charm_panel.js:328: container = Y.Node.create('<div id="test-container"></div>'); On 2012/11/30 20:37:47, benji wrote: > This node probably doesn't need the ID. Also, we have started hiding the test > container if the code under test still works with it hidden, that way the test > runner is a bit easier on the eyes. If not, we use "visibility: hidden" so as > to keep it as clean as possible. Done. https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcode335 test/test_charm_panel.js:335: charm_store = new juju.CharmStore( OK, thanks for your discussion on IRC. I have reformatted according to the new proposed rules. I'm mildly negative on those changes, though, as I'd prefer visibility to block closure over saving of vertical space. https://codereview.appspot.com/6842119/ -- https://code.launchpad.net/~bac/juju-gui/cs-filter/+merge/137050 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bac/juju-gui/cs-filter 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
