Merge lp:~gary/juju-gui/fixsavetext into lp:juju-gui/experimental

[Gary Poster]

Fix constraints save and add save confirmation

To QA, deploy a service with the inspector, and then reopen deployed inspector. In configuration and constraints, make changes and click save. The save button should now not change text when you click on it in the constraints tab. The save (and cancel) buttons only appear when there are changes to be saved. The fields correctly recognize when changes are reverted. When changes are saved, the field flashes green to show success.

checkboxes do not work properly in this branch: Rick is working on that. The cancel button does not work: that will be a separate branch.

For comment and discussion, code and UX. The green flash for fields on save is not as described by UX, but the desired display will be a lot more time consuming--and fragile, I am afraid--than what I have here. I am hoping this is a reasonable compromise.

I hope discussion will answer UX issues, as well as resolve what is appropriate for testing. I am a bit too focused on getting this out the door, so need some outside perspective.

https://codereview.appspot.com/13505044/

[Gary Poster (gary)]

A code review. Reviewers: mp+183996_code.launchpad.net,

Message:
Please take a look.

Description:
Fix constraints save and add save confirmation

To QA, deploy a service with the inspector, and then reopen deployed
inspector. In configuration and constraints, make changes and click
save. The save button should now not change text when you click on it
in the constraints tab. The save (and cancel) buttons only appear when
there are changes to be saved. The fields correctly recognize when
changes are reverted. When changes are saved, the field flashes green
to show success.

checkboxes do not work properly in this branch: Rick is working on that.
  The cancel button does not work: that will be a separate branch.

For comment and discussion, code and UX. The green flash for fields on
save is not as described by UX, but the desired display will be a lot
more time consuming--and fragile, I am afraid--than what I have here. I
am hoping this is a reasonable compromise.

I hope discussion will answer UX issues, as well as resolve what is
appropriate for testing. I am a bit too focused on getting this out the
door, so need some outside perspective.

https://code.launchpad.net/~gary/juju-gui/fixsavetext/+merge/183996

(do not edit description out of merge proposal)

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

Affected files +111, -15:
   A [revision details]
   M app/templates/service-configuration.partial
   M app/templates/service-constraints-viewlet.handlebars
   M app/views/databinding.js
   M app/views/inspector.js
   M lib/views/juju-inspector.less

[Jeff Pihach (hatch)]

Looking good but there is some duplication of work, plz see below.

https://codereview.appspot.com/13505044/diff/1/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13505044/diff/1/app/views/inspector.js#newcode1047
app/views/inspector.js:1047: if (!this.modifiedKeys) {
When a user changes the value of an input the databinding engine keeps
track of these changes via it's _storeChanged method on line 609 of
databinding.js. This method keeps track of these changes in each
viewlets _changedvalues method. It would be nice if you could use this
data set instead of trying to keep track of it yourself. If you need
reference to these values in this changed method you may need to pass it
into the method call on line 622.

https://codereview.appspot.com/13505044/

Unmerged revisions

1005. By Gary Poster on 2013-09-05

lint

1004. By Gary Poster on 2013-09-05

fix constraint save text; add confirmation of changes for constraints and config

=== modified file 'app/templates/service-configuration.partial'
--- app/templates/service-configuration.partial	2013-09-04 10:44:34 +0000
+++ app/templates/service-configuration.partial	2013-09-05 00:44:43 +0000
@@ -48,7 +48,7 @@
   </div>
 {{/settings}}
 {{#unless ghost}}
-<div class="yui3-g controls configuration-buttons">
+<div class="yui3-g controls closed configuration-buttons">
   <div class="yui3-u-1-2">
     <button class="cancel">Cancel</button>
   </div>

=== modified file 'app/templates/service-constraints-viewlet.handlebars'
--- app/templates/service-constraints-viewlet.handlebars	2013-08-06 20:49:47 +0000
+++ app/templates/service-constraints-viewlet.handlebars	2013-09-05 00:44:43 +0000
@@ -1,7 +1,7 @@
 <div class="view-container">
-  <legend>{{service.displayName}} constraints</legend>
+  <legend>Constraints for New Units</legend>
   {{> service-constraints-viewlet}}
-  <div class="controls">
+  <div class="controls closed">
     <button type="button" class="save-constraints confirm">Confirm</button>
   </div>
 </div>

=== modified file 'app/views/databinding.js'
--- app/views/databinding.js	2013-09-04 15:26:55 +0000
+++ app/views/databinding.js	2013-09-05 00:44:43 +0000
@@ -49,11 +49,11 @@
       },
       input: {
         'get': function(node) { return node.get('value');},
-        'set': function(node, value) { node.set('value', value);}
+        'set': function(node, value) { node.set('value', value || '');}
       },
       textarea: {
         'get': function(node) { return node.get('value');},
-        'set': function(node, value) { node.set('value', value);}
+        'set': function(node, value) { node.set('value', value || '');}
       },
       'default': {
         'get': function(node) { return node.get('text');},
@@ -607,7 +607,7 @@
       @param {Object} viewlet reference.
     */
     BindingEngine.prototype._storeChanged = function(e, viewlet) {
-      var key = e.currentTarget.getData('bind'),
+      var key = e.target.getData('bind'),
           save = true;
 
       viewlet._changedValues.forEach(function(value) {

=== modified file 'app/views/inspector.js'
--- app/views/inspector.js	2013-09-04 15:08:24 +0000
+++ app/views/inspector.js	2013-09-05 00:44:43 +0000
@@ -1065,12 +1065,22 @@
 
   var ConflictMixin = {
     'changed': function(node, key, field) {
-      var modelValue = this.model.get(key);
+      if (!this.modifiedKeys) {
+        this.modifiedKeys = {};
+      }
+      var modelValue = this.model.get(key) || '';
       var fieldValue = field.get(node);
+      var controls = this.container.one('.controls');
       if (modelValue !== fieldValue) {
-        node.addClass('modified');
+        node.replaceClass('change-saved', 'modified');
+        this.modifiedKeys[key] = true;
+        controls.removeClass('closed');
       } else {
         node.removeClass('modified');
+        delete this.modifiedKeys[key];
+        if (Object.keys(this.modifiedKeys).length === 0) {
+          controls.addClass('closed');
+        }
       }
     },
     'conflict': function(node, model, viewletName, resolve, binding) {
@@ -1131,11 +1141,27 @@
           '.conflict', this));
     },
     'unsyncedFields': function(dirtyFields) {
-      this.container.one('.controls .confirm').setHTML('Overwrite');
+      var node = this.container.one('.controls .confirm');
+      if (!node.getData('originalText')) {
+        node.setData('originalText', node.getHTML());
+      }
+      node.setHTML('Overwrite');
     },
     'syncedFields': function() {
-      this.container.one('.controls .confirm').setHTML('Save Changes');
-      this.container.all('.modified').removeClass('modified');
+      var controls = this.container.one('.controls');
+      var node = controls.one('.confirm');
+      var title = node.getData('originalText');
+      var modified = this.container.all('.modified');
+      if (title) {
+        node.setHTML(title);
+      }
+      modified.replaceClass('modified', 'change-saved');
+      // animationend handlers don't work reliably, once you hook them up
+      // with the associated custom browser names (e.g. webkitAnimationEnd)
+      // on the raw DOM node, so don't even bother with them.
+      Y.later(1000, modified, function() {
+        this.removeClass('change-saved');});
+      controls.addClass('closed');
     }
   };
 

=== modified file 'lib/views/juju-inspector.less'
--- lib/views/juju-inspector.less	2013-09-04 10:53:54 +0000
+++ lib/views/juju-inspector.less	2013-09-05 00:44:43 +0000
@@ -1,4 +1,5 @@
 @inspector-background-color: #221e1c;
+@inspector-changed-field: #dedede;
 @inspector-controls-height: 67px;
 @inspector-divider-bottom: rgba(0, 0, 0, 0.3);
 @inspector-divider-top: rgba(255, 255, 255, 0.05);
@@ -11,6 +12,54 @@
 @disabled-input-background-color: #727272;
 @disabled-input-text-color: #252525;
 
+@-webkit-keyframes pulse-green {
+    0% {
+      background-color: @inspector-changed-field;
+    }
+    10% {
+      background-color: #38b44a;
+    }
+    100% {
+      background-color: white;
+    }
+}
+
+@-moz-keyframes pulse-green {
+    0% {
+      background-color: @inspector-changed-field;
+    }
+    10% {
+      background-color: #38b44a;
+    }
+    100% {
+      background-color: white;
+    }
+}
+
+@-o-keyframes pulse-green {
+    0% {
+      background-color: @inspector-changed-field;
+    }
+    10% {
+      background-color: #38b44a;
+    }
+    100% {
+      background-color: white;
+    }
+}
+
+@keyframes pulse-green {
+    0% {
+      background-color: @inspector-changed-field;
+    }
+    10% {
+      background-color: #38b44a;
+    }
+    100% {
+      background-color: white;
+    }
+}
+
 .yui3-juju-inspector {
     input[type=text]::-ms-clear { display: none; }
 
@@ -208,6 +257,15 @@
             margin-top: 20px;
             border-top: 1px solid @inspector-divider-top;
             border-bottom: 1px solid @inspector-divider-bottom;
+            overflow: hidden;
+            height: 70px;
+            -moz-transition: .3s, height .3s;
+            -webkit-transition: .25s, height .3s;
+            transition: .3s, height .3s;
+
+            &.closed {
+                height: 0;
+            }
 
             button {
                 .create-border-radius(@border-radius);
@@ -661,7 +719,7 @@
                 .resolver {
                   color: black;
                   background-color: white;
-                  border: 1px solid #dedede;
+                  border: 1px solid @inspector-changed-field;
                   border-radius: 1px;
 
                   div {
@@ -669,17 +727,27 @@
                   }
                 }
                 .modified {
-                  background-color: #dedede;
+                  background-color: @inspector-changed-field;
                   background-image: url(/juju-ui/assets/images/field-changed.png);
                 }
                 .conflict-pending {
-                  background-color: #dedede;
+                  background-color: @inspector-changed-field;
                   background-image: url(/juju-ui/assets/images/field-conflict.png);
                 }
                 .conflict {
-                  background-color: #dedede;
+                  background-color: @inspector-changed-field;
                   background-image: url(/juju-ui/assets/images/field-resolved.png);
                  }
+                .change-saved {
+                  -webkit-animation: pulse-green 1s;
+                  -moz-animation: pulse-green 1s;
+                  -o-animation: pulse-green 1s;
+                  animation: pulse-green 1s;
+                  -webkit-animation-fill-mode: forwards;
+                  -moz-animation-fill-mode: forwards;
+                  -o-animation-fill-mode: forwards;
+                  animation-fill-mode: forwards;
+                }
             }
             .toggle {
                 padding: 10px @inspector-main-padding;