Conflict UX

Conflict UX

This branch creates the general flow of operations for the new conflict UX.
This version doesn't extract the code in a reusable fashion but includes it
directly inline to the viewlet. In the meantime more detailed 
styling can take place.

I find the outlined UX odd in practice and would rather skip the 'indication'
phase and directly offer the choices to resolve the conflict. It would almost
never be a good idea to proceed without seeing the real server value.

- I extracted the assets images/field-*.png used in the UI myself from the
PDF, better version might exist.

- The option popup is inline and doesn't float over existing content.

- The padding and asset sizes are only approximations.

- Checkboxes are not currently supported.

Other than that the impl seems to work ok.


To Test:
Deploy glance in the sandbox
Make a change in the db-user field by typing
	Verify you see a change indicator
Simulate a conflict from the console:
		s = app.db.services.item(0); c = s.get('config');c['db-user']='concon';s.set('config', c)
	Verify you see a conflict indicator
	Click the field 
	See both options
	Click and option
		See that it selects the value and clears the indicators

https://code.launchpad.net/~bcsaller/juju-gui/conflict-ux/+merge/178208

(do not edit description out of merge proposal)

[bcsaller, 2013-08-02 06:35:26]

Please take a look.

[bcsaller, 2013-08-02 06:44:02]

Please take a look.

[jeff.pihach, 2013-08-02 16:15:43]

Thanks for getting this branch out!

In attempting to QA I came across a bug which is only in this branch (even after
merging trunk) 

Deploy glance
Open inspector and go to settings
There are no settings

https://codereview.appspot.com/12323043/diff/3001/app/templates/viewlet-manag...
File app/templates/viewlet-manager.handlebars (left):

https://codereview.appspot.com/12323043/diff/3001/app/templates/viewlet-manag...
app/templates/viewlet-manager.handlebars:14: <!-- Expose control -->
Huw's latest branch adds the icons for this area so there will likely be
conflicts here when you merge in trunk.

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

https://codereview.appspot.com/12323043/diff/3001/app/views/databinding.js#ne...
app/views/databinding.js:546: var key = e.target.getData('bind'),
currentTarget is normalized by YUI so we will need to make sure target points to
the right place in IE

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

https://codereview.appspot.com/12323043/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1150: var handlers = [], watch = handlers.push;
newline for watch and i'd rename it to addHandler or storeHandler to make it a
little bit more self documenting

https://codereview.appspot.com/12323043/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1154: @method sendResolve
this is actually a function not a method ;-)

https://codereview.appspot.com/12323043/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1173: }, 1000);
Why the settimeout? why not immediately?

https://codereview.appspot.com/12323043/diff/3001/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (right):

https://codereview.appspot.com/12323043/diff/3001/lib/views/juju-inspector.le...
lib/views/juju-inspector.less:520: background-color: #dedede;
This color is repeated a lot it should probably be moved into a variable

https://codereview.appspot.com/12323043/diff/3001/lib/views/juju-inspector.le...
lib/views/juju-inspector.less:540: .resolved {
animation not transition?

animations aren't GPU accelerated

[bcsaller, 2013-08-02 17:25:33]

Please take a look.

[jeff.pihach, 2013-08-02 17:38:34]

LGTM
QA OK!

This is looking great, the dropdown and indicators could use some styling as
they are a little too far to the right and pop out of the dropdown but I like
the functionality and simplicity of it all.

[rharding, 2013-08-02 17:50:02]

Couple of comments below that are mostly small. 

Are there tests for this? I'm hesitant to ok without any. Writing tests helps
point out things that are hard to test and you mention a future refactoring
which will be hard to make sure works 100% without redoing all the qa/etc that
went into this all over again.

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

https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1171: setTimeout(function() {
can we not event hook this without the timeout?

https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1183: node.removeClass('conflict-pending');
I think this would work better if we had two classes. .conflict .pending and
then we'd be dealing with one or the other. We could just remove .pending, and
later remove .conflict and it'd be clear.

https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1187: node.getComputedStyle('width') + 4);
why do we need to +4 the width? can we not pad the element already?

[benjamin.saller, 2013-08-02 18:15:21]

On 2013/08/02 17:50:02, rharding wrote:
> Couple of comments below that are mostly small. 
> 
> Are there tests for this? I'm hesitant to ok without any. Writing tests helps
> point out things that are hard to test and you mention a future refactoring
> which will be hard to make sure works 100% without redoing all the qa/etc that
> went into this all over again.

The orig didn't have tests either. I want to add them and can work on that now,
but its sort of a small state machine and I wanted to make sure the QA looked
reasonable before encoding it all into tests as there is a lot of setup and
possible branches.

> 
> https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js
> File app/views/inspector.js (right):
> 
>
https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
> app/views/inspector.js:1171: setTimeout(function() {
> can we not event hook this without the timeout?
> 
>
https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
> app/views/inspector.js:1183: node.removeClass('conflict-pending');
> I think this would work better if we had two classes. .conflict .pending and
> then we'd be dealing with one or the other. We could just remove .pending, and
> later remove .conflict and it'd be clear.
> 
>
https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
> app/views/inspector.js:1187: node.getComputedStyle('width') + 4);
> why do we need to +4 the width? can we not pad the element already?

[benjamin.saller, 2013-08-02 18:18:54]

Thanks

https://codereview.appspot.com/12323043/diff/3001/app/templates/viewlet-manag...
File app/templates/viewlet-manager.handlebars (left):

https://codereview.appspot.com/12323043/diff/3001/app/templates/viewlet-manag...
app/templates/viewlet-manager.handlebars:14: <!-- Expose control -->
On 2013/08/02 16:15:43, jeff.pihach wrote:
> Huw's latest branch adds the icons for this area so there will likely be
> conflicts here when you merge in trunk.

and there were, but resolved in next push

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

https://codereview.appspot.com/12323043/diff/3001/app/views/databinding.js#ne...
app/views/databinding.js:546: var key = e.target.getData('bind'),
On 2013/08/02 16:15:43, jeff.pihach wrote:
> currentTarget is normalized by YUI so we will need to make sure target points
to
> the right place in IE

You make a good point.

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

https://codereview.appspot.com/12323043/diff/3001/app/views/inspector.js#newc...
app/views/inspector.js:1173: }, 1000);
On 2013/08/02 16:15:43, jeff.pihach wrote:
> Why the settimeout? why not immediately? 

The intention is to give the transition time to run, then remove it. Ideally a
helper that bound the 'transitioned' event that fires post transition would
remove the class.

https://codereview.appspot.com/12323043/diff/3001/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (right):

https://codereview.appspot.com/12323043/diff/3001/lib/views/juju-inspector.le...
lib/views/juju-inspector.less:540: .resolved {
On 2013/08/02 16:15:43, jeff.pihach wrote:
> animation not transition?
> 
> animations aren't GPU accelerated

Yeah, it should be a transition, but that part isn't really working at all yet
so removed.

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

https://codereview.appspot.com/12323043/diff/11001/app/views/inspector.js#new...
app/views/inspector.js:1187: node.getComputedStyle('width') + 4);
On 2013/08/02 17:50:03, rharding wrote:
> why do we need to +4 the width? can we not pad the element already?

Before I added the indicator icons I was using a border, this accounted for the
border width and can be removed now I expect, thanks.

[rharding, 2013-08-02 18:32:56]

I can't get the animation to work which is what the timeout is for. 

I think it'd be better if we could get the animation to trigger on the removal
of the .conflict so that it's auto triggered and we don't need a timeout to
remove a class added for that use.

If we can't get the animation working then lets at least remove the timeout code
for now and mark it as something to revisit.

While the code didn't previously have tests, I'd really prefer to not land code
without tests without an executive waiver.

[bcsaller, 2013-08-06 15:42:16]

Please take a look.

[rharding, 2013-08-07 15:03:43]

LGTM with the comments below, though I'm really uncomfortable with adding code
that doesn't support checkboxes when we know we want to support it. It should be
a test case so that we can help make sure we don't break checkbox support in the
future. 

Please add notes to the bug in the system that this branch will have to be
revisited as part of fixing that bug. Otherwise we'll end up with a bug in here
after the other bug is corrected.

Didn't QA, let me know if you need a QA as well.

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

https://codereview.appspot.com/12323043/diff/22001/app/views/inspector.js#new...
app/views/inspector.js:866: */
two spaces is two far over, same below.

https://codereview.appspot.com/12323043/diff/22001/app/views/inspector.js#new...
app/views/inspector.js:878: */
extra line

https://codereview.appspot.com/12323043/diff/22001/app/views/inspector.js#new...
app/views/inspector.js:902: node.removeClass('conflict-pending');
I still think this would work better as 

node.removeClass('pending');

and keying off .conflict.pending as two classes vs the remove/add work here.

https://codereview.appspot.com/12323043/diff/22001/test/test_conflict_ux.js
File test/test_conflict_ux.js (right):

https://codereview.appspot.com/12323043/diff/22001/test/test_conflict_ux.js#n...
test/test_conflict_ux.js:94: Y.Node.create(['<div
id="content">'].join('')).appendTo(container);
why go through the list of length one and join?

[benjamin.saller, 2013-08-07 15:15:47]

Thanks for the reviews. 

Made a note on the bug and in the MP about the checkboxes.

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

https://codereview.appspot.com/12323043/diff/22001/app/views/inspector.js#new...
app/views/inspector.js:902: node.removeClass('conflict-pending');
On 2013/08/07 15:03:44, rharding wrote:
> I still think this would work better as 
> 
> node.removeClass('pending');
> 
> and keying off .conflict.pending as two classes vs the remove/add work here.

I found pending ambiguous, maybe its a matter of taste.

https://codereview.appspot.com/12323043/diff/22001/test/test_conflict_ux.js
File test/test_conflict_ux.js (right):

https://codereview.appspot.com/12323043/diff/22001/test/test_conflict_ux.js#n...
test/test_conflict_ux.js:94: Y.Node.create(['<div
id="content">'].join('')).appendTo(container);
On 2013/08/07 15:03:44, rharding wrote:
> why go through the list of length one and join?

This is copied setup from another module. Agreed though.

[bcsaller, 2013-08-07 15:21:16]

*** Submitted:

Conflict UX

This branch creates the general flow of operations for the new conflict UX.
This version doesn't extract the code in a reusable fashion but includes it
directly inline to the viewlet. In the meantime more detailed 
styling can take place.

I find the outlined UX odd in practice and would rather skip the 'indication'
phase and directly offer the choices to resolve the conflict. It would almost
never be a good idea to proceed without seeing the real server value.

- I extracted the assets images/field-*.png used in the UI myself from the
PDF, better version might exist.

- The option popup is inline and doesn't float over existing content.

- The padding and asset sizes are only approximations.

- Checkboxes are not currently supported.

Other than that the impl seems to work ok.


To Test:
Deploy glance in the sandbox
Make a change in the db-user field by typing
	Verify you see a change indicator
Simulate a conflict from the console:
		s = app.db.services.item(0); c =
s.get('config');c['db-user']='concon';s.set('config', c)
	Verify you see a conflict indicator
	Click the field 
	See both options
	Click and option
		See that it selects the value and clears the indicators

R=jeff.pihach, rharding, benjamin.saller
CC=
https://codereview.appspot.com/12323043