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

Revision history for this message
Francesco Banconi (frankban) wrote :

Please take a look.

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>
On 2012/11/06 20:42:44, bac wrote:
> Why not use the new asset in this branch?

We discussed this with Gary and decided to create another card to
replace bootsrap icons with the new assets.
See https://bugs.launchpad.net/juju-gui/+bug/1075672

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:
On 2012/11/06 20:42:44, bac wrote:
> When selecting a configuration file, the name now overlaps with other
elements.
> This work probably should be a separate branch.

Agreed.

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;
On 2012/11/06 20:42:44, bac wrote:
> 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.

IIRC, I used the color picker to obtain these colors from the wireframe.

https://codereview.appspot.com/6819098/diff/1/lib/views/stylesheet.less#newcode871
lib/views/stylesheet.less:871: border-bottom: 1px solid #C2C2C2;
On 2012/11/06 20:42:44, bac wrote:
> 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.

Done. The border color is now a variable: @charm-panel-border-color.
Nice catch on this repetition, thanks.

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

« Back to merge proposal