Merge lp://staging/~gary/juju-gui/peekabooInspectorSave into lp://staging/juju-gui/experimental

Reviewers: mp+184688_code.launchpad.net,

Message:
Please take a look.

Description:
hide and show save controls

In the inspector, controls to save values are hidden when there are no
changes, and revealed when there are. Changes can come from saving the
form or from simply manually reverting values.

This also changes the "modified" asterisk to be removed when the user
manually reverts values.

To QA, create a service, and then open the configuration panel in the
post-deployment inspector. The save control is not shown. Make a
change to one of the fields, and the save controls appear. revert the
change(s) and the controls disappear again. Make a change and then save
the change, and the controls disappear. Do the same for the constraints
panel.

https://code.launchpad.net/~gary/juju-gui/peekabooInspectorSave/+merge/184688

(do not edit description out of merge proposal)

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

Affected files (+73, -32 lines):
   A [revision details]
   M app/templates/service-configuration.handlebars
   M app/templates/service-configuration.partial
   M app/templates/service-constraints-viewlet.handlebars
   M app/views/inspector.js
   M lib/views/juju-inspector.less
   M test/test_inspector_constraints.js

Code LGTM
QA minors
   should the cancel button in the save dialog reset the form? is that
not in this branch?
   I like the green flash better than the icon fading I think but is that
the expected UX here?

As an aside an issue I thought was fixed shows in this branch where the
scale up ux didn't fit in the space provided. Is that a missing trunk
merge or still pending in a Huw branch?

In general the QA was good, if those are non issues feel free to land.
You mentioned a revision to this though, if you want another look let me
know.

https://codereview.appspot.com/13368051/diff/1/test/test_inspector_constraints.js
File test/test_inspector_constraints.js (right):

https://codereview.appspot.com/13368051/diff/1/test/test_inspector_constraints.js#newcode246
test/test_inspector_constraints.js:246:
env.ws.msg(makeResponse(inspector.model, false));
Nice test

https://codereview.appspot.com/13368051/

*** Submitted:

hide and show save controls

In the inspector, controls to save values are hidden when there are no
changes, and revealed when there are. Changes can come from saving the
form or from simply manually reverting values.

This also changes the "modified" asterisk to be removed when the user
manually reverts values.

R=benjamin.saller
CC=
https://codereview.appspot.com/13368051

https://codereview.appspot.com/13368051/

Thank you very much for the review. Replies below.

On 09/10/2013 12:42 PM, Benjamin Saller wrote:
> Code LGTM
> QA minors
> should the cancel button in the save dialog reset the form? is that
> not in this branch?

Right, not in this branch.

> I like the green flash better than the icon fading I think but is that
> the expected UX here?

Luca gave me his OK. background images don't have CSS transition
support, and I wanted to keep things simple there.

>
> As an aside an issue I thought was fixed shows in this branch where the
> scale up ux didn't fit in the space provided. Is that a missing trunk
> merge or still pending in a Huw branch?

Still pending in Huw's branch, I think. Similarly, the destroy service
slider is see through and he's had a fix for that for awhile.

> In general the QA was good, if those are non issues feel free to land.
> You mentioned a revision to this though, if you want another look let me
> know.

Thank you. I'm landing with my additional changes, but to highlight the
changes in case you want to follow up, I put them below. The only
change I think would be slightly contentious is the last one in
app/views/databinding.js. Thank you again!

Gary

=== modified file 'app/views/databinding.js'
--- app/views/databinding.js 2013-09-09 13:55:02 +0000
+++ app/views/databinding.js 2013-09-10 18:47:05 +0000
@@ -59,7 +59,7 @@
           node.set('checked', this._normalizeValue(value));
         },
         'eq': function(node, value) {
- var currentValue = !! this.get(node);
+ var currentValue = !!this.get(node);
           return (this._normalizeValue(value) === currentValue);
         }
       }),
@@ -155,7 +155,6 @@

       @method deltaFromChange
       @param {Array} modelChangeKeys array of {String} keys that have
changed.
- @param {Y.EventFacade | Object} e an event change object.
       @return {Array} bindings array filtered by keys when present.
     */
     function deltaFromChange(modelChangeKeys) {
@@ -689,6 +688,10 @@
       if (viewlet.changed) {
         viewlet.changed(e.target, key, nodeHandler);
       }
+ // If there are no more changes, the viewlet has been synced
manually.
+ if (Object.keys(viewlet.changedValues).length === 0) {
+ viewlet.syncedFields();
+ }
     };

     /**

=== modified file 'app/views/inspector.js'
--- app/views/inspector.js 2013-09-09 20:56:00 +0000
+++ app/views/inspector.js 2013-09-10 18:38:56 +0000
@@ -1194,9 +1194,8 @@
         controls.removeClass('closed');
       } else {
         this._clearModified(node);
- if (Object.keys(this.changedValues).length === 0) {
- controls.addClass('closed');
- }
+ // Databinding calls syncedFields if there are no more changed
+ // values, and that method is responsible for closing the controls.
       }
     },

=== modified file 'lib/views/juju-inspector.less'
--- lib/views/juju-inspector.less 2013-09-09 20:56:00 +0000
+++ lib/views/juju-inspector.less 2013-09-10 18:38:05 +0000
@@ -250,7 +250,7 @@
             border-top: 1px solid @inspector-divider-top;
             border-bottom: 1px solid @inspector-divider-bottom;
             overflow: hid...