Fixes expose UI issue 1209097

Fixes expose UI issue 1209097

When clicking the expose toggle in the service
configuration inspector it would sometimes not
toggle even though the service was getting exposed.

https://code.launchpad.net/~hatch/juju-gui/expose-1209097/+merge/179522

(do not edit description out of merge proposal)

[jeff.pihach, 2013-08-09 19:20:50]

Please take a look.

[benjamin.saller, 2013-08-09 19:44:58]

I thought this wasn't a selector issue but that the callback was triggered down
the line threw an error as its context used to be the charm panel and we were
calling it from a viewlet and it got messed up, no?

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

https://codereview.appspot.com/12704043/diff/1/app/views/environment.js#newco...
app/views/environment.js:197: 'input#expose-toggle': { click: 'toggleExpose' },
This won't work in the long term, it has to be a class as we could have more
than one inspector at some point.

[jeff.pihach, 2013-08-09 20:23:40]

Thanks for the review, changed below.

The bug report is incorrect. The issue was that it was an event race condition
between the halting of bubbled events from the listener. The visual toggle is
simply caused by css related to the checkbox value.

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

https://codereview.appspot.com/12704043/diff/1/app/views/environment.js#newco...
app/views/environment.js:197: 'input#expose-toggle': { click: 'toggleExpose' },
Done, although there will still be side effects with multiple inspectors wrt the
lablel/checkbox id relation. We will have to generate partially dynamic id's for
them if the time comes.

[jeff.pihach, 2013-08-09 20:26:39]

Please take a look.

[benji, 2013-08-09 20:34:02]

LGTM with a couple of small comments/bikesheds.

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

https://codereview.appspot.com/12704043/diff/3002/app/templates/service-confi...
app/templates/service-configuration.handlebars:6: <input type="checkbox"
class="hidden-checkbox expose-toggle" id="expose-toggle">
It would be swell if this line were broken so as not to be > 79 characters long.

https://codereview.appspot.com/12704043/diff/3002/test/test_inspector_setting...
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/12704043/diff/3002/test/test_inspector_setting...
test/test_inspector_settings.js:123: var vMC =
inspector.viewletManager.get('container');
"vMC"... are you starting an internet-based rap group?

"container" would seem to be the established pattern.

[benjamin.saller, 2013-08-09 20:37:25]

LGTM, thanks

I think when we properly bind the events the viewlet's container this will make
more sense as well.

[jeff.pihach, 2013-08-09 20:41:19]

Thanks for the reviews - changes landing

[jeff.pihach, 2013-08-09 20:52:14]

*** Submitted:

Fixes expose UI issue 1209097

When clicking the expose toggle in the service
configuration inspector it would sometimes not
toggle even though the service was getting exposed.

R=benjamin.saller, benji
CC=
https://codereview.appspot.com/12704043