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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
