Merge lp:~frankban/juju-gui/bug-1074297-charm-panel-description into lp:juju-gui/experimental

[Francesco Banconi]

Charm panel description design.

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

[Francesco Banconi (frankban)]

Reviewers: mp+133097_code.launchpad.net,

Message:
Please take a look.

Description:
Charm panel description design.

This branch fixes the charm panel discription view
so that now it matches the visual design here:
https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqQk93MnZEWDlkU3c/edit
The change includes a `customize-scrollbar` Less mixin: the
Ubuntu-style scrollbar is now present in all the charm
panel views.
This branch does not include the icons (I have made
another card to replace bootstrap icons with customized ones,
and to revert the direction of chevron-[up|down].
The css changes also involve the charm configuration view,
slightly changed here to make it more similar to what can be seen in
https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqQXRDOUprMHNwTlk/edit.
However, consider that changing the config view is not a goal
of this branch.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6819098/

Affected files:
   A [revision details]
   M app/templates/charm-description.handlebars
   M app/templates/charm-pre-configuration.handlebars
   M lib/views/stylesheet.less
   M test/test_charm_panel.js

[Brad Crittenden (bac)]

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/

[lp:~frankban/juju-gui/bug-1074297-charm-panel-description]

230. By Francesco Banconi on 2012-11-07

Abstract the charm panel border column.

[Francesco Banconi (frankban)]

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/

[Francesco Banconi (frankban)]

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.

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

[Gary Poster (gary)]

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

https://codereview.appspot.com/6819098/diff/6001/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>
(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-description.handlebars#newcode10
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-description.handlebars#newcode23
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-description.handlebars#newcode60
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-description.handlebars#newcode63
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-configuration.handlebars
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-configuration.handlebars#newcode43
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 a...

[lp:~frankban/juju-gui/bug-1074297-charm-panel-description]

231. By Francesco Banconi on 2012-11-07

Merged source and resolved conflicts.

232. By Francesco Banconi on 2012-11-07

Restored previous position of file upload control in the charm config panel.

[Francesco Banconi (frankban)]

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

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

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-configuration.handlebars#newcode43
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...

[lp:~frankban/juju-gui/bug-1074297-charm-panel-description]

233. By Francesco Banconi on 2012-11-07

Fixes per UX review

[Francesco Banconi (frankban)]

*** Submitted:

Charm panel description design.

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

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

=== modified file 'app/templates/charm-description.handlebars'
--- app/templates/charm-description.handlebars	2012-11-05 21:51:27 +0000
+++ app/templates/charm-description.handlebars	2012-11-07 18:27:21 +0000
@@ -1,12 +1,18 @@
 <div>
-  <div class="charm-nav-back"><i class="icon-arrow-left icon-white"></i> Back</div>
+  <div class="charm-nav-back"><i class="icon-chevron-left"></i> Back</div>
 
   <div class="charm-description charm-panel">
-    <button class="btn btn-primary deploy"
-            data-url="{{id}}">Deploy</button>
-    <h3>{{package_name}}</h3>
+    <div id="charm-panel-head">
+      <h3>{{package_name}}</h3>
+      {{id}}
+    </div>
+
+    <div class="charm-panel-configure-buttons">
+      <button class="btn btn-primary deploy" data-url="{{id}}">Deploy</button>
+    </div>
+
     <div class="charm-section">
-      <h4><i class="icon-chevron-down"></i> Description</h4>
+      <h4 class="first"><i class="icon-chevron-down"></i> Description</h4>
       <div class="collapsible">
         <h5>Summary</h5>
         <p>{{summary}}</p>
@@ -14,11 +20,9 @@
         <p>{{series}}</p>
         <h5>Description</h5>
         <div>{{description}}</div>
+        {{#if owner}}
         <h5>Owner</h5>
-        {{#if owner}}
         <p>{{owner}}</p>
-        {{else}}
-        <p>[Official charm]</p>
         {{/if}}
         {{#if store_revision}}
         <h5>Store Revision</h5>
@@ -26,6 +30,7 @@
         {{/if}}
       </div>
     </div>
+
     {{#any requires provides}}
     <div class="charm-section">
       <h4><i class="icon-chevron-up"></i> Interfaces</h4>
@@ -49,11 +54,13 @@
       </div>
     </div>
     {{/any}}
+
     {{#if last_change}}
     <div class="charm-section">
-      <h4><i class="icon-chevron-up"></i> Last Change</h4>
+      <h4><i class="icon-chevron-up"></i> Change Log</h4>
       <div class="collapsible">
         {{#with last_change}}
+        <h5>Last Change</h5>
         <div>{{dateformat created "%F"}}</div>
         <div>{{committer}}</div>
         <div class="commitmessage">{{message}}</div>
@@ -61,13 +68,15 @@
       </div>
     </div>
     {{/if}}
+
     {{#any requires provides}}
     <div class="charm-section">
-      <h4><i class="icon-chevron-up"></i>Related Charms</h4>
+      <h4><i class="icon-chevron-up"></i> Related Charms</h4>
       <div class="collapsible" id="related-charms">
         Loading...
       </div>
     </div>
     {{/any}}
+
   </div>
 </div>
 
=== modified file 'app/templates/charm-pre-configuration.handlebars'
--- app/templates/charm-pre-configuration.handlebars	2012-10-26 10:44:21 +0000
+++ app/templates/charm-pre-configuration.handlebars	2012-11-07 18:27:21 +0000
@@ -52,11 +52,11 @@
           </div>
         </div>
         <div class="charm-settings">
-        <form>
-          <div id="service-config" class="collapsible">
-            {{> service-config}}
-          </div>
-        </form>
+          <form>
+            <div id="service-config" class="collapsible">
+              {{> service-config}}
+            </div>
+          </form>
         </div>
       </div>
       {{/if}}
 
=== modified file 'lib/views/stylesheet.less'
--- lib/views/stylesheet.less	2012-11-07 13:18:31 +0000
+++ lib/views/stylesheet.less	2012-11-07 18:27:21 +0000
@@ -1,5 +1,6 @@
 /* Processed with LESS from lib/views/stylesheet.less */
 @font-family: 'Ubuntu', 'Helvetica Neue', Helvetica, Arial, sans-serif;
+@font-family-medium: 'Ubuntu Medium', 'Helvetica Neue', Helvetica, Arial, sans-serif;
 
 /* Define common colors. */
 @label-color: #292929;
@@ -12,8 +13,8 @@
 @charm-panel-cancel-button-color: #b8b8b8;
 @charm-panel-cancel-button-bottom-gradient: #a3a39f;
 @charm-panel-cancel-button-shadow: #515050;
-@charm-panel-separator-color: #aeaeae;
 @charm-panel-background-color: #eeeeee;
+@charm-panel-border-color: #BEBEBE;
 @navbar-color: #2D2D2D!important;
 
 body {
@@ -127,9 +128,10 @@
                     margin: 0 5px;
                 }
                 input {
+                    height: 23px;
                     width: 128px;
                     background: url(/juju-ui/assets/images/searchbox_icon.png)
-                                no-repeat 2px 1px whiteSmoke;
+                                no-repeat 4px 4px whiteSmoke;
                     margin: 0 10px;
                     padding: 3px 3px 3px 26px;
                     font-style: italic;
@@ -772,7 +774,20 @@
     }
 }
 
+.customize-scrollbar {
+    ::-webkit-scrollbar {
+        width: 4px;
+    }
+    ::-webkit-scrollbar-track {
+        -webkit-box-shadow: inset 1px 0 3px #989A9C;
+    }
+    ::-webkit-scrollbar-thumb {
+        -webkit-box-shadow: inset 4px 0 3px #D74613;
+    }
+}
+
 #juju-search-charm-panel {
+    .customize-scrollbar;
     display: block;
     z-index: 999;
     padding: 0;
@@ -820,7 +835,8 @@
         }
     }
     .charm-panel-configure-buttons {
-      border-bottom: 1px solid @charm-panel-separator-color;
+      background-color: #FFFFFF;
+      border-bottom: 1px solid @charm-panel-border-color;
       height: 52px;
       padding-left: 8px;
       .btn {
@@ -832,43 +848,62 @@
         height: 500px;
     }
     .charm-nav-back {
-        background-color: black;
-        text-transform: uppercase;
-        color: white;
-        font-weight: bold;
+        border-top: 1px solid #DEDEDE;
+        border-bottom: 1px solid @charm-panel-border-color;
+        color: @charm-panel-deploy-button-color;
         cursor: pointer;
+        font-size: 14px;
+        padding: 7px 4px;
     }
     .charm-panel {
+        background-color: @charm-panel-background-color;
+        height: 432px;
+        overflow-y: auto;
+        overflow-x: hidden;
         h3 {
-            text-transform: capitalize;
-            color: blue;
-            font-weight: bold;
-            margin-top: 1ex;
-            margin-bottom: 1ex;
+            color: @label-color;
+            font-size: 22px;
+            font-weight: normal;
+            padding: 5px 0px 2px 0px;
         }
         h4 {
-            border-bottom: 1px solid @label-color;
+            color: @label-color;
             cursor: pointer;
-            margin-top: 1ex;
-            margin-bottom: 1ex;
+            border-top: 1px solid #FFFFFF;
+            border-bottom: 1px solid @charm-panel-border-color;
+            font-family: @font-family-medium;
+            font-weight: normal;
+            padding: 8px 12px;
             i {
                 float: right;
             }
+            &.first {
+                border-top: 1px solid #E0DEDE;
+            }
         }
         h5 {
-            margin-top: 1ex;
-            &:first-child {
-                margin-top: 0;
-            }
-        }
-        height: 432px;
-        overflow-y: auto;
-        overflow-x: hidden;
+            color: @label-color;
+            font-family: @font-family-medium;
+            font-weight: normal;
+            margin-top: 11px;
+        }
+        #charm-panel-head {
+            border-top: 1px solid #FFFFFF;
+            border-bottom: 1px solid @charm-panel-border-color;
+            padding: 1ex 12px 10px 12px;
+        }
         .collapsible {
+            background-color: #E7E7E7;
+            line-height: 16px;
             overflow: hidden;
+            padding: 0px 12px;
+            :last-child {
+                margin-bottom: 11px;
+            }
         }
         &.charm-description {
-            padding: 1ex 1em;
+            color: #58595B;
+            font-size: 12px;
             .charm-section {
                 .commitmessage {
                     padding: 0.8ex 0 1ex 1em;
@@ -883,14 +918,13 @@
             }
         }
         &.config-variant {
-            padding: 1ex 1em;
-            background-color: @charm-panel-background-color;
             font-size: 14px;
+
             .control-group {
                 width: auto;
                 margin-bottom: 0;
                 margin-top: 1ex;
-                padding-bottom: 0;
+                padding: 0px 12px;
                 clear: both;
             }
             .control-description {
 
=== modified file 'test/test_charm_panel.js'
--- test/test_charm_panel.js	2012-11-06 02:07:38 +0000
+++ test/test_charm_panel.js	2012-11-07 18:27:21 +0000
@@ -115,7 +115,7 @@
     field.set('value', 'aaa');
     field.simulate('keydown', { keyCode: ENTER });
     node.one('a.charm-detail').simulate('click');
-    node.one('.charm-description > h3').get('text').trim()
+    node.one('.charm-description h3').get('text').trim()
       .should.equal('membase');
   });
 
@@ -212,15 +212,10 @@
         { container: container, app: app, model: charm,
           charmStore: charm_store }).render(),
         html = container.one('.charm-description'),
-        description_div = html.one('.charm-section div'),
-        interface_div = html.one('div.charm-section:nth-of-type(2)'),
-        last_change_div = html.one('div.charm-section:nth-of-type(3)'),
-        related_div = html.one('div.charm-section:nth-of-type(4)');
+        description_div = html.one('.charm-section div');
     html.one('h3').get('text').trim().should.equal('mysql');
     description_div.getStyle('height').should.not.equal('0px');
-    var _ = expect(interface_div).to.not.exist;
-    _ = expect(last_change_div).to.not.exist;
-    _ = expect(related_div).to.not.exist;
+    html.all('div.charm-section').size().should.equal(1);
   });
 
   it('can render fuller charm', function() {
@@ -240,10 +235,11 @@
         { container: container, app: app, model: charm, charms: charms,
           charmStore: charm_store }).render(),
         html = container.one('.charm-description'),
-        description_div = html.one('.charm-section div'),
-        interface_div = html.one('div.charm-section:nth-of-type(2) div'),
-        last_change_div = html.one('div.charm-section:nth-of-type(3) div'),
-        related_div = html.one('div.charm-section:nth-of-type(4)');
+        sections = html.all('.charm-section'),
+        description_div = sections.item(0).one('div'),
+        interface_div = sections.item(1).one('div'),
+        last_change_div = sections.item(2).one('div'),
+        related_div = sections.item(3).one('div');
     description_div.get('text').should.contain('A DB');
     interface_div.getStyle('height').should.equal('0px');
     interface_div.get('text').should.contain('munin');
@@ -268,7 +264,8 @@
           charmStore: charm_store }).render(),
         html = container.one('.charm-description'),
         // We use the last change div.
-        section_container = html.one('div.charm-section:nth-of-type(3)');
+        section_container = html.one('div.charm-section:last-child');
+        //section_container = html.one('div.charm-section:nth-of-type(3)');
     section_container.one('div').getStyle('height').should.equal('0px');
     assert(section_container.one('h4 i').hasClass('icon-chevron-up'));
     section_container.one('h4').simulate('click');