Merge lp:~frankban/juju-gui/bug-1075679-charm-config-panel into lp:juju-gui/experimental

[Francesco Banconi]

Charm config panel visual design fixes.

The charm configuration panel now should match UX wireframe.
This branch also contains fixes to the configuration
section hiding when a config file is uploaded.
Also introduced LESS mixins for border radii and box shadows.
Added "docstrings" to relevant charm panel methods.

https://codereview.appspot.com/6827066/

[Francesco Banconi (frankban)]

Reviewers: mp+133657_code.launchpad.net,

Message:
Please take a look.

Description:
Charm config panel visual design fixes.

The charm configuration panel now should match UX wireframe.
This branch also contains fixes to the configuration
section hiding when a config file is uploaded.
Also introduced LESS mixins for border radii and box shadows.
Added "docstrings" to relevant charm panel methods.

https://code.launchpad.net/~frankban/juju-gui/bug-1075679-charm-config-panel/+merge/133657

(do not edit description out of merge proposal)

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

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

[Brad Crittenden (bac)]

Nice changes Francesco.

I think the config file button still needs some attention. I find it
odd that it isn't part of the collapsible div and remains visible when
Service Settings is collapsed. Did you discuss that UX with Matt or
Jovan? Of course I defer to them but think it currently looks funny.

Also, when a file is chosen, Service Settings collapses but the chevron
still points down. Further you can click on it and the chevron changes
but nothing else happens. I guess it'd be nice to collapse, set the
chevron to up, and lock it in place.

https://codereview.appspot.com/6827066/diff/1/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6827066/diff/1/app/views/charm-panel.js#newcode601
app/views/charm-panel.js:601: */
Nice, thanks.

https://codereview.appspot.com/6827066/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6827066/diff/1/lib/views/stylesheet.less#newcode43
lib/views/stylesheet.less:43:
Thanks! Maybe 'set-' instead of 'create-' for both?

https://codereview.appspot.com/6827066/diff/1/lib/views/stylesheet.less#newcode987
lib/views/stylesheet.less:987: padding-left: 5px;
The vis design shows this lined up with the other elements and the charm
icon up top. I think the padding-left should be 11px.

Good to see you and Matt discussing it.

And as I mentioned in IRC we should decide on one value and apply it to
all charm panels, abstracting it into a LESS variable.

https://codereview.appspot.com/6827066/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/6827066/diff/1/undocumented#oldcode82
undocumented:82: app/views/charm-panel.js render
yay

https://codereview.appspot.com/6827066/

[lp:~frankban/juju-gui/bug-1075679-charm-config-panel]

237. By Francesco Banconi on 2012-11-09

Fixes per review.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/6827066/

[Gary Poster (gary)]

Great changes, Francesco! No suggestions or requests. Thanks!

Gary

https://codereview.appspot.com/6827066/diff/2002/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6827066/diff/2002/app/views/charm-panel.js#newcode600
app/views/charm-panel.js:600: * @return {undefined} Dispatches only.
Awesome. Thank you for this and the other docs

https://codereview.appspot.com/6827066/diff/2002/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6827066/diff/2002/lib/views/stylesheet.less#newcode1
lib/views/stylesheet.less:1: /* Processed with LESS from
lib/views/stylesheet.less */
Great changes here, Francesco.

https://codereview.appspot.com/6827066/diff/2002/undocumented
File undocumented (left):

https://codereview.appspot.com/6827066/diff/2002/undocumented#oldcode81
undocumented:81: app/views/charm-panel.js onFileRemove
Thank you!

https://codereview.appspot.com/6827066/

[Francesco Banconi (frankban)]

*** Submitted:

Charm config panel visual design fixes.

The charm configuration panel now should match UX wireframe.
This branch also contains fixes to the configuration
section hiding when a config file is uploaded.
Also introduced LESS mixins for border radii and box shadows.
Added "docstrings" to relevant charm panel methods.

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

https://codereview.appspot.com/6827066/

=== modified file 'app/templates/charm-pre-configuration.handlebars'
--- app/templates/charm-pre-configuration.handlebars	2012-11-07 14:40:38 +0000
+++ app/templates/charm-pre-configuration.handlebars	2012-11-09 14:33:29 +0000
@@ -44,16 +44,16 @@
       <!-- Service configuration form -->
       <div class="charm-section">
         <h4>Service Settings<i class="icon-chevron-down"></i></h4>
-        <div class="config-file-upload control-group">
-          <input class="config-file-upload-widget" type="file">
-          <div>
-            <span class="config-file-upload-overlay">Use configuration file</span>
-            <span class="config-file-name"></span>
+        <div class="collapsible">
+          <div class="config-file-upload control-group">
+            <input class="config-file-upload-widget" type="file">
+            <div>
+              <span class="config-file-upload-overlay">Use configuration file</span>
+              <span class="config-file-name"></span>
+            </div>
           </div>
-        </div>
-        <div class="charm-settings">
-          <form>
-            <div id="service-config" class="collapsible">
+          <form class="charm-settings">
+            <div id="service-config">
               {{> service-config}}
             </div>
           </form>

=== modified file 'app/views/charm-panel.js'
--- app/views/charm-panel.js	2012-11-07 13:12:18 +0000
+++ app/views/charm-panel.js	2012-11-09 14:33:29 +0000
@@ -591,7 +591,14 @@
           this.tooltip.hide();
           delete this.tooltip.field;
         },
-        /** Pass clicks on the overlay on to the correct recipient. */
+        /**
+         * Pass clicks on the overlay on to the correct recipient.
+         * The recipient can be the upload widget or the file remove one.
+         *
+         * @method onOverlayClick
+         * @param {Object} evt An event object.
+         * @return {undefined} Dispatches only.
+         */
         onOverlayClick: function(evt) {
           var container = this.get('container');
           if (this.configFileContent) {
@@ -600,6 +607,14 @@
             container.one('.config-file-upload-widget').getDOMNode().click();
           }
         },
+        /**
+         * Handle the file upload click event.
+         * Call onFileLoaded or onFileError if an error occurs during upload.
+         *
+         * @method onFileChange
+         * @param {Object} evt An event object.
+         * @return {undefined} Mutates only.
+         */
         onFileChange: function(evt) {
           var container = this.get('container');
           console.log('onFileChange:', evt);
@@ -613,6 +628,13 @@
           container.one('.config-file-upload-overlay')
             .setContent('Remove file');
         },
+        /**
+         * Handle the file remove click event.
+         * Restore the file upload widget on click.
+         *
+         * @method onFileRemove
+         * @return {undefined} Mutates only.
+         */
         onFileRemove: function() {
           var container = this.get('container');
           this.configFileContent = null;
@@ -624,9 +646,20 @@
           this.fileInput.replace(Y.Node.create('<input type="file"/>')
                                  .addClass('config-file-upload-widget'));
           this.fileInput = container.one('.config-file-upload-widget');
-          container.one('.config-file-upload-overlay')
-            .setContent('Use configuration file');
+          var overlay = container.one('.config-file-upload-overlay');
+          overlay.setContent('Use configuration file');
+          // Ensure the charm section height is correctly restored.
+          overlay.ancestor('.collapsible')
+            .show('sizeIn', {duration: 0.25, width: null});
         },
+        /**
+         * Callback called when a file is correctly uploaded.
+         * Hide the charm configuration section.
+         *
+         * @method onFileLoaded
+         * @param {Object} evt An event object.
+         * @return {undefined} Mutates only.
+         */
         onFileLoaded: function(evt) {
           this.configFileContent = evt.target.result;
 
@@ -645,6 +678,14 @@
           }
           this.get('container').one('.charm-settings').hide();
         },
+        /**
+         * Callback called when an error occurs during file upload.
+         * Hide the charm configuration section.
+         *
+         * @method onFileError
+         * @param {Object} evt An event object (with a "target.error" attr).
+         * @return {undefined} Mutates only.
+         */
         onFileError: function(evt) {
           console.log('onFileError:', evt);
           var msg;

=== modified file 'lib/views/stylesheet.less'
--- lib/views/stylesheet.less	2012-11-08 15:50:13 +0000
+++ lib/views/stylesheet.less	2012-11-09 14:33:29 +0000
@@ -17,6 +17,7 @@
 @charm-panel-background-color: #eeeeee;
 @charm-panel-border-color: #BEBEBE;
 @charm-paneel-border-top-color: #E0DEDE;
+@charm-panel-padding-left: 10px;
 @navbar-color: #2D2D2D!important;
 
 body {
@@ -29,6 +30,18 @@
     font-family: @font-family;
 }
 
+.create-border-radius(@radius) {
+    -moz-border-radius: @radius;
+    -webkit-border-radius: @radius;
+    border-radius: @radius;
+}
+
+.create-box-shadow(@arguments) {
+    -moz-box-shadow: @arguments;
+    -webkit-box-shadow: @arguments;
+    box-shadow: @arguments;
+}
+
 .create-button(@gradient-start, @gradient-end, @shadow-color, @v-pos) {
     @blur: 2px;
     background-image: -ms-linear-gradient(top, @gradient-start, @gradient-end);
@@ -38,21 +51,15 @@
     background-image: -moz-linear-gradient(top, @gradient-start, @gradient-end);
     background-image: linear-gradient(top, @gradient-start, @gradient-end);
     background-repeat: repeat-x;
-    -moz-box-shadow: 0 (@v-pos + 3) @blur -@blur @shadow-color inset,
-                        2px 0 3px -2px @shadow-color inset, -2px 0 3px -2px @shadow-color inset;
-    -webkit-box-shadow: 0 (@v-pos + 3) @blur -@blur @shadow-color inset,
-                           2px 0 3px -2px @shadow-color inset, -2px 0 3px -2px @shadow-color inset;
-    box-shadow: 0 (@v-pos + 3) @blur -@blur @shadow-color inset,
-                   2px 0 3px -2px @shadow-color inset, -2px 0 3px -2px @shadow-color inset;
-    @border-radii: ~"6px / 7px";
-    -moz-border-radius: @border-radii;
-    -webkit-border-radius: @border-radii;
-    border-radius: @border-radii;
+    // Using a variable here because LESS strips commas in mixin args.
+    @box-shadow: 0 (@v-pos + 3) @blur -@blur @shadow-color inset,
+                 2px 0 3px -2px @shadow-color inset,
+                 -2px 0 3px -2px @shadow-color inset;
+    .create-box-shadow(@box-shadow);
+    .create-border-radius(~"6px / 7px");
     border: none;
 }
 
-
-
 i.sprite {
     display: inline-block;
     vertical-align: middle;
@@ -219,15 +226,8 @@
     display: none;
     background-color: @background_color;
     color: #fdf6e3;
-
-    border-radius: @border_radius;
-    -moz-border-radius: @border_radius;
-    -webkit-border-radius: @border_radius;
-
-    box-shadow: -2px 4px 4px 0 rgba(0, 0, 0, 0.5);
-    -moz-box-shadow: -2px 4px 4px 0 rgba(0, 0, 0, 0.5);
-    -webkit-box-shadow: -2px 4px 4px 0 rgba(0, 0, 0, 0.5);
-
+    .create-border-radius(@border_radius);
+    .create-box-shadow(-2px 4px 4px 0 rgba(0, 0, 0, 0.5));
     top: 0;
     left: 0;
     position: absolute;
@@ -271,9 +271,7 @@
             cursor: pointer;
             line-height: 32px;
             padding: 5px 5px 5px 5px;
-            border-radius: @border_radius;
-            -moz-border-radius: @border_radius;
-            -webkit-border-radius: @border_radius;
+            .create-border-radius(@border_radius);
 
             &.view-service {
                 background-image: url(/juju-ui/assets/images/icons/icon_noshadow_view.png);
@@ -379,9 +377,7 @@
 
 .unit (@min_height: 20px, @border_radius:3px) {
     padding: 5px;
-    -webkit-border-radius: @border_radius;
-    -moz-border-radius: @border_radius;
-    border-radius: @border_radius;
+    .create-border-radius(@border_radius);
     margin-bottom: 0px;
     min-height: @min_height;
     cursor: pointer;
@@ -731,12 +727,8 @@
     background: white;
     padding: 1.6em;
     border: solid black 2px;
-    -webkit-border-radius: 6px;
-    -moz-border-radius: 6px;
-    border-radius: 6px;
-    -webkit-box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3);
-    -moz-box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3);
-    box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3);
+    .create-border-radius(6px);
+    .create-box-shadow(0 3px 7px rgba(0, 0, 0, 0.3));
     -webkit-background-clip: padding-box;
     -moz-background-clip: padding-box;
     background-clip: padding-box;
@@ -844,9 +836,10 @@
       background-color: #FFFFFF;
       border-bottom: 1px solid @charm-panel-border-color;
       height: 52px;
-      padding-left: 8px;
+      padding-left: @charm-panel-padding-left;
       .btn {
           margin-top: 12px;
+          width: 67px;
       }
     }
     .search-charm-inner {
@@ -879,7 +872,7 @@
             border-bottom: 1px solid @charm-panel-border-color;
             font-family: @font-family-medium;
             font-weight: normal;
-            padding: 8px 12px;
+            padding: 8px @charm-panel-padding-left;
             i {
                 float: right;
             }
@@ -896,13 +889,13 @@
         #charm-panel-head {
             border-top: 1px solid #FFFFFF;
             border-bottom: 1px solid @charm-panel-border-color;
-            padding: 1ex 12px 10px 12px;
+            padding: 1ex 10px 10px @charm-panel-padding-left;
         }
         .collapsible {
             background-color: #E7E7E7;
             line-height: 16px;
             overflow: hidden;
-            padding: 0px 12px;
+            padding: 0px @charm-panel-padding-left;
             :last-child {
                 margin-bottom: 11px;
             }
@@ -910,6 +903,9 @@
         &.charm-description {
             color: #58595B;
             font-size: 12px;
+            .btn.deploy {
+                margin-left: 0;
+            }
             .charm-section {
                 .commitmessage {
                     padding: 0.8ex 0 1ex 1em;
@@ -926,11 +922,16 @@
         &.config-variant {
             font-size: 14px;
             border-top: 1px solid @charm-paneel-border-top-color;
+            .charm-entry {
+                padding: 5px @charm-panel-padding-left;
+                &:nth-of-type(2) {
+                    margin-bottom: 15px;
+                }
+            }
             .control-group {
                 width: auto;
                 margin-bottom: 0;
                 margin-top: 1ex;
-                padding: 0px 12px;
                 clear: both;
             }
             .control-description {
@@ -946,6 +947,23 @@
             .controls {
                 margin-left: auto;
                 margin-right: 0;
+                input[type=text] {
+                    color: @text-entry-color;
+                    font-style: italic;
+                    font-size: 12px;
+                    height: 15px;
+                    width: 250px;
+                    margin: 0;
+                    .create-border-radius(6px);
+                    // Using a variable here because LESS strips commas in
+                    // mixin args.
+                    @box-shadow: 0 1px 3px #959595 inset, 0 1px 0 white;
+                    .create-box-shadow(@box-shadow);
+                }
+                input[type=checkbox] {
+                    margin-left: 1ex;
+                    margin-top: 0.7ex;
+                }
             }
             .config-field {
                 font-size: 16px;
@@ -955,6 +973,8 @@
                 position: relative;
                 width: 252px;
                 height: 25px;
+                margin-bottom: 20px;
+                margin-top: 20px;
             }
             .config-file-upload-widget {
                 position: absolute;
@@ -965,6 +985,8 @@
             }
             .config-file-upload-overlay {
                 background: url(/juju-ui/assets/images/getfile_button.png) no-repeat;
+                color: #4c4c4c;
+                font-family: @font-family-medium;
                 position: absolute;
                 top: 0;
                 left: 0;
@@ -985,12 +1007,6 @@
                 width: auto;
                 z-index: 2;
             }
-            .controls.boolean {
-                input {
-                    margin-left: 1ex;
-                    margin-top: 0.7ex;
-                }
-            }
             .btn {
                  margin-top: 5px;
             }
@@ -1005,7 +1021,7 @@
                 font-weight: normal;
                 font-size: 16px;
                 text-transform: capitalize;
-                padding-left: 11px;
+                padding-left: @charm-panel-padding-left;
                 background-color: #CBCBCB;
                 border-top: 1px solid #989898;
             }
@@ -1013,7 +1029,7 @@
                 margin-bottom: 0;
             }
             .charm-entry {
-                padding: 11px;
+                padding: 11px @charm-panel-padding-left;
                 border-top: 1px solid #FFFFFF;
                 border-bottom: 1px solid #BDBDBD;
                 &:first-child {
@@ -1271,9 +1287,7 @@
     background-color: white;
     border: solid 1px black;
     padding: 0.2em 0.5em 0.3em;
-    -webkit-border-radius: 3px;
-    -moz-border-radius: 3px;
-    border-radius: 3px;
+    .create-border-radius(3px);
     min-width: 20px;
     max-width: 400px;
 }
@@ -1281,11 +1295,11 @@
 .charm-panel-configure {
   background-image: url(/juju-ui/assets/images/configure-cog.png);
   background-repeat: no-repeat;
-  background-position: 230px 42px;
+  background-position: 230px 38px;
   border-top: 2px solid #dd4814;
   background-color: #2F2A27;
-  height: 93px;
-  padding-left: 8px;
+  height: 90px;
+  padding-left: @charm-panel-padding-left;
   .title {
     font-weight: lighter;
     font-size: 22px;

=== modified file 'undocumented'
--- undocumented	2012-11-08 20:11:46 +0000
+++ undocumented	2012-11-09 14:33:29 +0000
@@ -75,10 +75,6 @@
 app/views/charm-panel.js mouseenter
 app/views/charm-panel.js mouseleave
 app/views/charm-panel.js onCharmDeployClicked
-app/views/charm-panel.js onFileChange
-app/views/charm-panel.js onFileError
-app/views/charm-panel.js onFileLoaded
-app/views/charm-panel.js onFileRemove
 app/views/charm-panel.js render
 app/views/charm-panel.js setDefaultSeries
 app/views/charm-panel.js setPanel