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

Revision history for this message
Brad Crittenden (bac) wrote :

Nice branch Francesco. I've made a few comments but nothing serious.

I do think the configuration file stuff needs to be fixed or possibly
deferred to another branch.

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

https://codereview.appspot.com/6819098/diff/1/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>
Why not use the new asset in this branch?

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

https://codereview.appspot.com/6819098/diff/1/app/templates/charm-pre-configuration.handlebars#newcode50
app/templates/charm-pre-configuration.handlebars:50:
When selecting a configuration file, the name now overlaps with other
elements. This work probably should be a separate branch.

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

https://codereview.appspot.com/6819098/diff/1/lib/views/stylesheet.less#newcode850
lib/views/stylesheet.less:850: border-bottom: 1px solid #C2C2C2;
Where did these colors come from? They don't seem to match the ones in
the assets provided (charm_head2_div.png, charm_detail_title_div.png,
etc). They do *look* really good, though.

https://codereview.appspot.com/6819098/diff/1/lib/views/stylesheet.less#newcode871
lib/views/stylesheet.less:871: border-bottom: 1px solid #C2C2C2;
It would be nice to abstract these re-used colors, if possible in a
readable manner. I cannot believe we have so many different shades of
gray in this stylesheet, but that's what has been specified.

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

https://codereview.appspot.com/6819098/diff/1/test/test_charm_panel.js#newcode234
test/test_charm_panel.js:234:
description_div.get('text').should.contain('A DB');
Nice simplification.

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

« Back to merge proposal