yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

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/

-- 
https://code.launchpad.net/~frankban/juju-gui/bug-1074297-charm-panel-description/+merge/133097
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/juju-gui/bug-1074297-charm-panel-description into lp:juju-gui.