Service expose should use on-off slider

Service expose should use on-off slider

The "service expose" feature should use on-off slider 
(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqS19SYWQ2MzU3cFU/edit) 
instead of the current button.

https://code.launchpad.net/~tveronezi/juju-gui/service-expose-on-off-slider/+merge/129679

(do not edit description out of merge proposal)

[thiago, 2012-10-15 14:07:50]

Please take a look.

[thiago, 2012-10-15 14:32:26]

Please take a look.

[frankban, 2012-10-15 15:30:22]

This is a trainee review from Nicola and me.
Looks good Thiago, just a few comments that you will find below.

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
app/templates/service-header.partial:20: <div
class="inline"><span>Expose&nbsp;&nbsp;</span></div>
We could also draw the slider below its label, so that the visualization is
similar to what is displayed in "Unit count" (on the right). In the wireframes
the slider is present in the footer right corner: nonetheless the current
placement also seems nice.

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

https://codereview.appspot.com/6693043/diff/3001/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:874: font: Ubuntu regular 12px #ffffff;
A space is missing between position and absolute. The same below in .off.

[gary.poster, 2012-10-15 16:39:46]

As we discussed with Nick and Matt, I'm concerned about the fact that this UX
implies sliding to me and yet it does not support sliding.  I wonder if
mousedown rather than click will be a better choice, as Nick questioned, and I
am interested to see how that experiment goes.

Thank you

Gary

https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
app/templates/service-header.partial:20: <div
class="inline"><span>Expose&nbsp;&nbsp;</span></div>
Can we get rid of the nbsps and do that with styling instead?

https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
app/templates/service-header.partial:24:
src="/juju-ui/assets/images/slider_on.png" />
low priority, but might as well add alt tags to this and the "off" image.

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
app/templates/service-header.partial:20: <div
class="inline"><span>Expose&nbsp;&nbsp;</span></div>
On 2012/10/15 15:30:22, francesco.banconi wrote:
> We could also draw the slider below its label, so that the visualization is
> similar to what is displayed in "Unit count" (on the right). In the wireframes
> the slider is present in the footer right corner: nonetheless the current
> placement also seems nice.
Yeah, it is on the bottom right in the visual designs now too
(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqMXpQZWtYWHJrNms/edit).
 I think that might be a new change.  We can have that in another branch,
though.

[thiago, 2012-10-15 17:14:15]

https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
app/templates/service-header.partial:20: <div
class="inline"><span>Expose&nbsp;&nbsp;</span></div>
On 2012/10/15 16:39:47, gary.poster wrote:
> Can we get rid of the nbsps and do that with styling instead?

Done.

https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
app/templates/service-header.partial:24:
src="/juju-ui/assets/images/slider_on.png" />
On 2012/10/15 16:39:47, gary.poster wrote:
> low priority, but might as well add alt tags to this and the "off" image.

Done.

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
app/templates/service-header.partial:20: <div
class="inline"><span>Expose&nbsp;&nbsp;</span></div>
On 2012/10/15 16:39:47, gary.poster wrote:
> On 2012/10/15 15:30:22, francesco.banconi wrote:
> > We could also draw the slider below its label, so that the visualization is
> > similar to what is displayed in "Unit count" (on the right). In the
wireframes
> > the slider is present in the footer right corner: nonetheless the current
> > placement also seems nice.
> Yeah, it is on the bottom right in the visual designs now too
>
(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqMXpQZWtYWHJrNms/edit).
>  I think that might be a new change.  We can have that in another branch,
> though.

Exactly. I will do this in a new branch.

https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
app/templates/service-header.partial:20: <div
class="inline"><span>Expose&nbsp;&nbsp;</span></div>
On 2012/10/15 15:30:22, francesco.banconi wrote:
> We could also draw the slider below its label, so that the visualization is
> similar to what is displayed in "Unit count" (on the right). In the wireframes
> the slider is present in the footer right corner: nonetheless the current
> placement also seems nice.

I will do this in another card

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

https://codereview.appspot.com/6693043/diff/3001/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:874: font: Ubuntu regular 12px #ffffff;
On 2012/10/15 15:30:22, francesco.banconi wrote:
> A space is missing between position and absolute. The same below in .off.

Done.

[thiago, 2012-10-15 17:40:30]

Please take a look.

[thiago, 2012-10-15 17:42:57]

I've changed the listener to mousedown. It seems better. At least the user does
not have time to try to drag the button this way. :O)


On 2012/10/15 16:39:46, gary.poster wrote:
> As we discussed with Nick and Matt, I'm concerned about the fact that this UX
> implies sliding to me and yet it does not support sliding.  I wonder if
> mousedown rather than click will be a better choice, as Nick questioned, and I
> am interested to see how that experiment goes.
> 
> Thank you
> 
> Gary
> 
>
https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
> File app/templates/service-header.partial (right):
> 
>
https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
> app/templates/service-header.partial:20: <div
> class="inline"><span>Expose&nbsp;&nbsp;</span></div>
> Can we get rid of the nbsps and do that with styling instead?
> 
>
https://codereview.appspot.com/6693043/diff/1/app/templates/service-header.pa...
> app/templates/service-header.partial:24:
> src="/juju-ui/assets/images/slider_on.png" />
> low priority, but might as well add alt tags to this and the "off" image.
> 
>
https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
> File app/templates/service-header.partial (right):
> 
>
https://codereview.appspot.com/6693043/diff/3001/app/templates/service-header...
> app/templates/service-header.partial:20: <div
> class="inline"><span>Expose&nbsp;&nbsp;</span></div>
> On 2012/10/15 15:30:22, francesco.banconi wrote:
> > We could also draw the slider below its label, so that the visualization is
> > similar to what is displayed in "Unit count" (on the right). In the
wireframes
> > the slider is present in the footer right corner: nonetheless the current
> > placement also seems nice.
> Yeah, it is on the bottom right in the visual designs now too
>
(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqMXpQZWtYWHJrNms/edit).
>  I think that might be a new change.  We can have that in another branch,
> though.

[gary.poster, 2012-10-15 17:47:24]

Please try that spacing again.  I don't like nbsp; used as spacing, and I also
don't like random empty element used for spacing.  This is what the box model is
for, unless I don't understand some subtlety of what is going on here.

https://codereview.appspot.com/6693043/diff/10001/app/templates/service-heade...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/10001/app/templates/service-heade...
app/templates/service-header.partial:21: <div class="empty inline"></div>
no, that still adds random junk in our code.  I had a margin-right or a
padding-right in mind.  Please get rid of this and the associated css.

[thiago, 2012-10-15 18:18:23]

https://codereview.appspot.com/6693043/diff/10001/app/templates/service-heade...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/10001/app/templates/service-heade...
app/templates/service-header.partial:21: <div class="empty inline"></div>
On 2012/10/15 17:47:25, gary.poster wrote:
> no, that still adds random junk in our code.  I had a margin-right or a
> padding-right in mind.  Please get rid of this and the associated css.

Done.

[thiago, 2012-10-15 18:19:00]

Please take a look.

[gary.poster, 2012-10-15 18:27:18]

My goal is to have us add as little unnecessary bits as possible.

https://codereview.appspot.com/6693043/diff/2002/app/templates/service-header...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/2002/app/templates/service-header...
app/templates/service-header.partial:20: <div class="inline
expose-label"><span>Expose</span></div>
Do you really need this?  Can't your selector be
.juju-service-container-header-expose span or something else, without adding
more stuff here?

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

https://codereview.appspot.com/6693043/diff/2002/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:872: .expose-label {
Just even "div" is good enough, yeah?

[thiago, 2012-10-15 19:00:33]

https://codereview.appspot.com/6693043/diff/2002/app/templates/service-header...
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6693043/diff/2002/app/templates/service-header...
app/templates/service-header.partial:20: <div class="inline
expose-label"><span>Expose</span></div>
On 2012/10/15 18:27:18, gary.poster wrote:
> Do you really need this?  Can't your selector be
> .juju-service-container-header-expose span or something else, without adding
> more stuff here?

Done.

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

https://codereview.appspot.com/6693043/diff/2002/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:872: .expose-label {
On 2012/10/15 18:27:18, gary.poster wrote:
> Just even "div" is good enough, yeah?

first-child

[thiago, 2012-10-15 19:03:01]

Please take a look.

[gary.poster, 2012-10-15 20:13:27]

Cool, thanks Thiago.  Approved.  Please ask hazmat to review and merge.

Gary

[thiago, 2012-10-17 18:29:16]

*** Submitted:

Service expose should use on-off slider

The "service expose" feature should use on-off slider 
(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqS19SYWQ2MzU3cFU/edit)

instead of the current button.

R=francesco.banconi, gary.poster
CC=
https://codereview.appspot.com/6693043