yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

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.