Update conflict ux per updated design.

Update conflict ux per updated design.

- design images: https://www.dropbox.com/sh/kuzzisylxrozkbe/iUR0SPSIXL#/
- Update checkboxes to be a slider widget, two levels tall
- Display the True/False value and keep it in sync
- Update the Expose/Use default setting checkboxes to match the updated design
- Update conflict settings for the conflict-pending with the new smaller image

https://code.launchpad.net/~rharding/juju-gui/update-conflict-ux/+merge/184804

(do not edit description out of merge proposal)

[rharding, 2013-09-11 14:13:42]

Please take a look.

[rharding, 2013-09-11 14:24:17]

Added inline comments to most things.

https://codereview.appspot.com/13516044/diff/3001/app/templates/ghost-config-...
File app/templates/ghost-config-viewlet.handlebars (right):

https://codereview.appspot.com/13516044/diff/3001/app/templates/ghost-config-...
app/templates/ghost-config-viewlet.handlebars:33: <label>Use the default
configuration?</label>
use label to get the proper styling of the heading of the section.

https://codereview.appspot.com/13516044/diff/3001/app/templates/ghost-config-...
app/templates/ghost-config-viewlet.handlebars:41: <div
class="settings-description"></div>
this is required to clear the floats of the toggle and the textvalue.

https://codereview.appspot.com/13516044/diff/3001/app/templates/service-confi...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/13516044/diff/3001/app/templates/service-confi...
app/templates/service-configuration.handlebars:5: <label
for="expose-toggle">Expose this service?</label>
Sync the expose UI with the config ones.

https://codereview.appspot.com/13516044/diff/3001/app/templates/service-confi...
app/templates/service-configuration.handlebars:8: data-bind="exposed">
notice that the data-bind is on the div and not the input in this case so we can
avoid changed/conflict ux issues. It's not really 'changed' from the checkbox,
but a manual event on that checkbox that was pre-existing.

https://codereview.appspot.com/13516044/diff/3001/app/templates/service-confi...
app/templates/service-configuration.handlebars:19: data-skipconflictux=true
this is new since we have two fields bound to the same model attribute. We added
the skip to prevent two changed/conflict indicators.

https://codereview.appspot.com/13516044/diff/3001/app/templates/service-confi...
File app/templates/service-configuration.partial (right):

https://codereview.appspot.com/13516044/diff/3001/app/templates/service-confi...
app/templates/service-configuration.partial:4: <div class="toggle
settings-wrapper">
moved inside so that the outer toggle class could be added.

https://codereview.appspot.com/13516044/diff/3001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13516044/diff/3001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:220: if (input) {
in the case of the things like exposed/etc they don't have the id we match on
and this can come up undefined.

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

https://codereview.appspot.com/13516044/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1091: var n = node.ancestor('.toggle').one('.modified');
with the new ux, the modified is on the label now vs the input field.

https://codereview.appspot.com/13516044/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1132: var n =
node.ancestor('.toggle').one('.conflict-pending');
and the conflict indicator is prefixed before the boolean toggle field

https://codereview.appspot.com/13516044/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1196: }
if a field is marked to skip conflictux then we don't indicate changed or
conflict-pending on it.

https://codereview.appspot.com/13516044/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1223: if (resolver) {
checkboxes no longer have any resolver input. You resolve a conflict by clicking
on the indicator or by toggling the value again.

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlet-manager.js
File app/views/viewlet-manager.js (right):

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlet-manager.j...
app/views/viewlet-manager.js:86: events: [],
Add support for viewlets to have events they bind on render.

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlet-manager.j...
app/views/viewlet-manager.js:162: },
and make the default destroy clean any of those up.

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlets/service-...
File app/views/viewlets/service-config.js (right):

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlets/service-...
app/views/viewlets/service-config.js:34: bindings: {
moved bindings up to the top so it's consistent with the other viewlets. Also
removed '' around those that don't need it. Syntax highlighting ftw.

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlets/service-...
app/views/viewlets/service-config.js:36: 'update': function(node, val) {
since the data-bound element is the div, we need to sync the checkbox for
everything else to work right.

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlets/service-...
app/views/viewlets/service-config.js:102: charm.get('options'),
service.get('config'));
In order to sync the values correctly we need to take the current
values/defaults into account.

https://codereview.appspot.com/13516044/diff/3001/app/views/viewlets/service-...
app/views/viewlets/service-config.js:126: 'resizing-textarea'
:!sort ftw

[rharding, 2013-09-11 14:44:11]

Please take a look.

[jeff.pihach, 2013-09-11 15:32:53]

Thanks for putting in the time to get all the odd issues ironed out with this!

Most of my comments are pretty small fixes with a small change to the
skipconflict stuff and the big one - using the current convention for binding
events.

Great idea adding the review comments but I feel like a number of them, the ones
which didn't simply indicate a reason for the change, could be moved into the
code to explain what is happening. 

The review notes are a great idea that we should probably implement team wide
but it's unfortunate that they don't carry forward on Patch Sets.

https://codereview.appspot.com/13516044/diff/11001/app/templates/ghost-config...
File app/templates/ghost-config-viewlet.handlebars (right):

https://codereview.appspot.com/13516044/diff/11001/app/templates/ghost-config...
app/templates/ghost-config-viewlet.handlebars:50: <div
class="settings-description"></div>
As per your comment - this things function is very confusiong, could you add a
comment here as per what is going on so this doesn't get removed
unintentionally.

https://codereview.appspot.com/13516044/diff/11001/app/templates/service-conf...
File app/templates/service-configuration.partial (right):

https://codereview.appspot.com/13516044/diff/11001/app/templates/service-conf...
app/templates/service-configuration.partial:7: <div class="toggle
settings-wrapper">
I see you moved this inside - because we usually use the indentation to find the
closing div's I'd like to see this settings-wrapper divs indented to line up
with their closing tag outside of the ifblocks

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

https://codereview.appspot.com/13516044/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1194: if (node.getData('skipconflictux')) {
This feels like it should be a feature of the databinding and not the viewlet.
I'd move this check into the databinding and not even call the changed method.

https://codereview.appspot.com/13516044/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1212: if (node.getData('skipconflictux')) {
Same goes for here.

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlet-manager.js
File app/views/viewlet-manager.js (right):

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlet-manager....
app/views/viewlet-manager.js:79: The list of events the viewlet has bound so we
can clean up on destroy.
It is not the viewlets responsibility to manage it's events with the current
conventions. This event handling stuff should be removed in leu of the current
convention of the environments.js file

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
File app/views/viewlets/service-config.js (right):

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
app/views/viewlets/service-config.js:57:
node.ancestor('.toggle').one('.textvalue').set('text',
Why do you need to set the html of the .textvalue even though it's databound?
Shouldn't it just update in the 'else' of the ifblock below ?

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
app/views/viewlets/service-config.js:76: _bindDOMEvents: function() {
As mentioned previously this breaks the event binding convention and introduces
yet another convention which won't be used even after the event refactoring. I'd
like this refactored to match the current event conventions

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
File app/views/viewlets/service-ghost.js (right):

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
app/views/viewlets/service-ghost.js:62: _bindDOMEvents: function() {
See other comments about the event binding conventions.

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
app/views/viewlets/service-ghost.js:77: * @method rener
typo

https://codereview.appspot.com/13516044/diff/11001/test/test_ghost_inspector.js
File test/test_ghost_inspector.js (right):

https://codereview.appspot.com/13516044/diff/11001/test/test_ghost_inspector....
test/test_ghost_inspector.js:288: input.simulate('click');
This test is actually impossible to do in real life because we click the label
not the checkbox. So a minor change in the selector should be all that's
required.

https://codereview.appspot.com/13516044/diff/11001/test/test_inspector_settin...
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/13516044/diff/11001/test/test_inspector_settin...
test/test_inspector_settings.js:160:
container.one('input[name=debug]').simulate('click');
Same as in the other suite - we can't actually do this, this will need to be
changed to the label.

[bac, 2013-09-11 18:14:59]

LGTM pending resolution of Jeff's issues.

https://codereview.appspot.com/13516044/diff/11001/app/templates/service-conf...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/13516044/diff/11001/app/templates/service-conf...
app/templates/service-configuration.handlebars:12: data-skipconflictux=true
data-skipconflictux is one of the most difficult to read names ever, though it
sounds mildly Latin.

https://codereview.appspot.com/13516044/diff/11001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13516044/diff/11001/app/views/ghost-inspector....
app/views/ghost-inspector.js:221: if (input.get('type') !== 'checkbox') {
If both cases are handled, I really prefer to have the conditional be for
equality not inequality.  (I know that is old code you're just wrapping.)

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

https://codereview.appspot.com/13516044/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1193: // toggle checkbox UI.
That last phrase doesn't make sense to me.

https://codereview.appspot.com/13516044/diff/11001/test/test_conflict_ux.js
File test/test_conflict_ux.js (right):

https://codereview.appspot.com/13516044/diff/11001/test/test_conflict_ux.js#n...
test/test_conflict_ux.js:163: // See that it got the proper style added
// Show that the modified class was not added.

https://codereview.appspot.com/13516044/diff/11001/test/test_ghost_inspector.js
File test/test_ghost_inspector.js (right):

https://codereview.appspot.com/13516044/diff/11001/test/test_ghost_inspector....
test/test_ghost_inspector.js:257: // Basic sanity checks of the rendering
Add punctuation to comments please.

https://codereview.appspot.com/13516044/diff/11001/test/test_ghost_inspector....
test/test_ghost_inspector.js:261: 'debug checkbox is not disabled'
Nice assert messages.

https://codereview.appspot.com/13516044/diff/11001/test/test_inspector_settin...
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/13516044/diff/11001/test/test_inspector_settin...
test/test_inspector_settings.js:140: // One boolean checkbox even though two
show up (one is the expose button)
Please add a period if this first line is a complete sentence.

[rharding, 2013-09-12 13:11:54]

Update on the way.

https://codereview.appspot.com/13516044/diff/11001/app/templates/ghost-config...
File app/templates/ghost-config-viewlet.handlebars (right):

https://codereview.appspot.com/13516044/diff/11001/app/templates/ghost-config...
app/templates/ghost-config-viewlet.handlebars:50: <div
class="settings-description"></div>
On 2013/09/11 15:32:53, jeff.pihach wrote:
> As per your comment - this things function is very confusiong, could you add a
> comment here as per what is going on so this doesn't get removed
> unintentionally. 

You're right. I've changed this to use a global css class clearall so that its
intention is more clear along with the comment.

https://codereview.appspot.com/13516044/diff/11001/app/templates/service-conf...
File app/templates/service-configuration.partial (right):

https://codereview.appspot.com/13516044/diff/11001/app/templates/service-conf...
app/templates/service-configuration.partial:7: <div class="toggle
settings-wrapper">
On 2013/09/11 15:32:53, jeff.pihach wrote:
> I see you moved this inside - because we usually use the indentation to find
the
> closing div's I'd like to see this settings-wrapper divs indented to line up
> with their closing tag outside of the ifblocks

Moved outside again and added a second isBool check. It looks cleaner. I debated
back/forth.

https://codereview.appspot.com/13516044/diff/11001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13516044/diff/11001/app/views/ghost-inspector....
app/views/ghost-inspector.js:221: if (input.get('type') !== 'checkbox') {
On 2013/09/11 18:14:59, bac wrote:
> If both cases are handled, I really prefer to have the conditional be for
> equality not inequality.  (I know that is old code you're just wrapping.)

Agree 100%. Moved.

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

https://codereview.appspot.com/13516044/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1194: if (node.getData('skipconflictux')) {
On 2013/09/11 15:32:53, jeff.pihach wrote:
> This feels like it should be a feature of the databinding and not the viewlet.
> I'd move this check into the databinding and not even call the changed method.

I'll disagree here. The conflict UX is handled via a mixin and has it's own
tests, etc. It doesn't make sense to me for the html to say "databinding, don't
show conflict stuff". It makes more sense for it to speak directly.

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
File app/views/viewlets/service-config.js (right):

https://codereview.appspot.com/13516044/diff/11001/app/views/viewlets/service...
app/views/viewlets/service-config.js:57:
node.ancestor('.toggle').one('.textvalue').set('text',
On 2013/09/11 15:32:53, jeff.pihach wrote:
> Why do you need to set the html of the .textvalue even though it's databound?
> Shouldn't it just update in the 'else' of the ifblock below ?

So the issue here I found is that databinding would call update, but would not
set the value itself. So I added it here. If it's supposed to work differently
I'd be up for pairing on what it's supposed to do vs is doing, however since
lots of things use the update method I assumed this was correct.

[rharding, 2013-09-12 13:14:01]

Please take a look.

[rharding, 2013-09-12 13:20:10]

Please take a look.

[jeff.pihach, 2013-09-12 13:25:31]

Thanks for putting in the work to refactor this! 

A couple things were left in that were unused so remove those and LGTM!

https://codereview.appspot.com/13516044/diff/32001/app/views/viewlet-manager.js
File app/views/viewlet-manager.js (right):

https://codereview.appspot.com/13516044/diff/32001/app/views/viewlet-manager....
app/views/viewlet-manager.js:79: The list of events the viewlet has bound so we
can clean up on destroy.
This is not used and can be removed

https://codereview.appspot.com/13516044/diff/32001/app/views/viewlet-manager....
app/views/viewlet-manager.js:158: this.events.forEach(function(ev) {
the body of this can be removed as well.

[bac, 2013-09-12 13:27:19]

LGTM

[rharding, 2013-09-12 13:39:42]

*** Submitted:

Update conflict ux per updated design.

- design images: https://www.dropbox.com/sh/kuzzisylxrozkbe/iUR0SPSIXL#/
- Update checkboxes to be a slider widget, two levels tall
- Display the True/False value and keep it in sync
- Update the Expose/Use default setting checkboxes to match the updated design
- Update conflict settings for the conflict-pending with the new smaller image

R=jeff.pihach, bac
CC=
https://codereview.appspot.com/13516044