Code review comment for lp:~frankban/juju-gui/bug-1074297-charm-panel-description

Hey Francesco. This looks very good. Thank you.

I am chatty in the line-by-line comments but I only ask for one or two
things, and you may ignore them if you disagree.

You will almost certainly want to merge trunk by hand before you land
this, since (thanks to your review) I have landed the "related charms"
addition to the description pane, and you'll need to make sure that it
has necessary changes to look good with your improvements.

Thanks again,

Gary

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars
File app/templates/charm-description.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode2
app/templates/charm-description.handlebars:2: <div
class="charm-nav-back"><i class="icon-chevron-left"></i> Back</div>
(As we discussed, we have a separate card for converting these to the
desired assets.)

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode10
app/templates/charm-description.handlebars:10: <div
class="charm-panel-configure-buttons">
yay, we actually reused a class name in the charm panel! will wonders
never cease? :-) thank you for being thoughtful with this

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode23
app/templates/charm-description.handlebars:23: {{#if owner}}
I think I have a similar block that instead draws "[charmers]" for the
owner name. I think I prefer your solution. I don't think we have a
specification for this. If we do, go with that; otherwise, make your
own choice, and I suggest sticking with what you have here.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode60
app/templates/charm-description.handlebars:60: <h4><i
class="icon-chevron-up"></i> Change Log</h4>
I had intentionally diverged from the "Change Log" text because I
thought a single change was not a log. This is what was specified
though, you are right. +1 on this change.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode63
app/templates/charm-description.handlebars:63: <h5>Last Change</h5>
Oh, I see. That's fine.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-configuration.handlebars
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-configuration.handlebars#newcode43
app/templates/charm-pre-configuration.handlebars:43: <div
class="config-file-upload control-group">
Moving this into the service settings was a conscious decision by the
devs who did it, I think, even though the visual design had it here.
The configuration file replaces the configuration form, and IIRC makes
it hide.

In this particular case, I suggest moving it back where it was before,
in the "Service Settings." OTOH, if you disagree for any reason, you
have the visual design on your side, so I'm happy for this change to win
in that case. :-)

It also doesn't look quite right--it needs some left padding--but I'm
figuring you want to hold off on that change for another branch.

https://codereview.appspot.com/6819098/diff/6001/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6819098/diff/6001/lib/views/stylesheet.less#newcode785
lib/views/stylesheet.less:785: }
This looks good, and I love the fact that we don't have to have JS for
this. OTOH, though I was reconciled to not having FF and trying to get
agreement on that limitation, I thought we could have IE 10: after some
research that you probably also did, it looks like that CSS is a bad
idea (it does not allow changing width, and reportedly using the IE
scriollbar color CSS forces older-style scrollbars, per
http://stackoverflow.com/questions/4053220/css-scrollbar-width). I
guess let's make this change and see if anyone complains--in which case
I suppose we'll have to try the JS thing.

https://codereview.appspot.com/6819098/diff/6001/lib/views/stylesheet.less#newcode789
lib/views/stylesheet.less:789: .customize-scrollbar;
oh, that's cool. I hadn't really registered the LESS mixin approach.

https://codereview.appspot.com/6819098/diff/6001/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6819098/diff/6001/test/test_charm_panel.js#newcode216
test/test_charm_panel.js:216:
html.all('div.charm-section').size().should.equal(1);
Nicer, thanks

https://codereview.appspot.com/6819098/diff/6001/test/test_charm_panel.js#newcode230
test/test_charm_panel.js:230: sections = html.all('.charm-section'),
ah, that's nicer, thanks

https://codereview.appspot.com/6819098/diff/6001/test/test_charm_panel.js#newcode253
test/test_charm_panel.js:253: section_container =
html.one('div.charm-section:last-child');
This might break with my branch...or might not. :-)

https://codereview.appspot.com/6819098/