Charm panel description design.

Charm panel description design.



https://code.launchpad.net/~frankban/juju-gui/bug-1074297-charm-panel-description/+merge/133097

(do not edit description out of merge proposal)

[frankban, 2012-11-06 16:41:27]

Please take a look.

[bac, 2012-11-06 20:42:44]

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...
File app/templates/charm-description.handlebars (right):

https://codereview.appspot.com/6819098/diff/1/app/templates/charm-description...
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-configu...
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/6819098/diff/1/app/templates/charm-pre-configu...
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#newco...
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#newco...
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#newcod...
test/test_charm_panel.js:234: description_div.get('text').should.contain('A
DB');
Nice simplification.

[frankban, 2012-11-07 10:23:58]

Please take a look.

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

https://codereview.appspot.com/6819098/diff/1/app/templates/charm-description...
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-configu...
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/6819098/diff/1/app/templates/charm-pre-configu...
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#newco...
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#newco...
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.

[frankban, 2012-11-07 10:26:37]

Thanks for the review Brad.

We have a card for the charm config panel, and, since it is possible to reuse
some of this work, I will take care of that task once this branch is landed.

[gary.poster, 2012-11-07 13:58:59]

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-descript...
File app/templates/charm-description.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-descript...
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-descript...
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-descript...
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-descript...
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-descript...
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-conf...
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-conf...
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#ne...
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#ne...
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#new...
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#new...
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#new...
test/test_charm_panel.js:253: section_container =
html.one('div.charm-section:last-child');
This might break with my branch...or might not. :-)

[frankban, 2012-11-07 14:42:13]

Thanks for the review Gary.
Already merged trunk and resolved some conflicts encountered.
Now I will add fixes per UX review and land this branch.

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

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-descript...
app/templates/charm-description.handlebars:2: <div class="charm-nav-back"><i
class="icon-chevron-left"></i> Back</div>
On 2012/11/07 13:58:59, gary.poster wrote:
> (As we discussed, we have a separate card for converting these to the desired
> assets.)

Cool.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-descript...
app/templates/charm-description.handlebars:10: <div
class="charm-panel-configure-buttons">
On 2012/11/07 13:58:59, gary.poster wrote:
> yay, we actually reused a class name in the charm panel!  will wonders never
> cease? :-) thank you for being thoughtful with this

:-) I have to fix other things per UX review, however this seems to be fine. \o/

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-descript...
app/templates/charm-description.handlebars:23: {{#if owner}}
On 2012/11/07 13:58:59, gary.poster wrote:
> 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.

Done.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-descript...
app/templates/charm-description.handlebars:60: <h4><i
class="icon-chevron-up"></i> Change Log</h4>
On 2012/11/07 13:58:59, gary.poster wrote:
> 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.

Thanks, and I agree with you that one change is not a log.

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

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-conf...
app/templates/charm-pre-configuration.handlebars:43: <div
class="config-file-upload control-group">
On 2012/11/07 13:58:59, gary.poster wrote:
> 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.

Restored the previous position, I missed the "config file hides the form"
behavior. This config panel looks ugly but hopefully will be fixed in 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#ne...
lib/views/stylesheet.less:785: }
On 2012/11/07 13:58:59, gary.poster wrote:
> 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.

Agreed.

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#new...
test/test_charm_panel.js:253: section_container =
html.one('div.charm-section:last-child');
On 2012/11/07 13:58:59, gary.poster wrote:
> This might break with my branch...or might not. :-)

It didn't!

[frankban, 2012-11-07 18:39:21]

*** Submitted:

Charm panel description design.

R=bac, gary.poster
CC=
https://codereview.appspot.com/6819098