Ghost default configuration toggle

Ghost default configuration toggle

This implements the 'use default configuration' toggle in the
ghost inspector. It's active by default and all inputs and 
text areas are disabled when it's active. Clicking the toggle
will re-enable all inputs. If you have changed some options
then click the toggle it will reset all values to their 
defaults.

https://code.launchpad.net/~hatch/juju-gui/use-def-config/+merge/177967

(do not edit description out of merge proposal)

[jeff.pihach, 2013-07-31 23:09:19]

Please take a look.

[jeff.pihach, 2013-08-01 18:06:07]

Please take a look.

[j.c.sackett, 2013-08-01 18:13:09]

This looks LGTM-y; I have one comment below regarding the repeated ifs in the
template.

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

https://codereview.appspot.com/12205043/diff/13002/app/templates/service-conf...
app/templates/service-configuration.partial:25: <textarea data-bind="{{#if
../../../ghost}}options{{else}}config{{/if}}.{{name}}" class="config-field"
id="input-{{name}}" name="{{name}}" {{#if
../../../ghost}}disabled{{/if}}>{{value}}</textarea>
I'm seeing a repeated pattern here; mightn't this better be a single handlebars
helper?

[rharding, 2013-08-01 18:19:10]

Up for a chat on this. The branch removes some callbacks but I don't see the
code removed. There's no tests. I've got a few questions as well.

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

https://codereview.appspot.com/12205043/diff/13002/app/templates/service-conf...
app/templates/service-configuration.partial:13: id="input-{{name}}"
data-bind="{{#if ../../ghost}}options{{else}}config{{/if}}.{{name}}" {{value}}
{{#if ../../ghost}}disabled{{/if}}/>
Can you newline before the data-bind and after the value? It's already split and
should be safe.

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

https://codereview.appspot.com/12205043/diff/13002/app/views/ghost-inspector....
app/views/ghost-inspector.js:179: serviceConfig = container.one(
can we call this a node? container is generally a node, but serviceConfig is
vague.

https://codereview.appspot.com/12205043/diff/13002/app/views/ghost-inspector....
app/views/ghost-inspector.js:185: if (useDefaults) {
should this be updating the data or is there some data-binding magic that will
return changed fields back to their original default settings?

https://codereview.appspot.com/12205043/diff/13002/app/views/ghost-inspector....
app/views/ghost-inspector.js:202: viewletContainer.one('#input-' +
key).set('value', newVal);
does this work for checkbox elements in the config as well?

https://codereview.appspot.com/12205043/diff/13002/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/12205043/diff/13002/app/views/utils.js#newcode...
app/views/utils.js:1437: Y.Handlebars.registerHelper('debugger', function(value)
{
what did value add here? This is the context of the block correct?

Maybe add a doc to the doc block on the new param? Is there a less generic name
that value?

If this is just to inspect a value maybe the block can be something that will
console.log(value)? Maybe as a helepr?

[rharding, 2013-08-01 18:51:22]

Another comment.

https://codereview.appspot.com/12205043/diff/13002/app/views/environment.js
File app/views/environment.js (left):

https://codereview.appspot.com/12205043/diff/13002/app/views/environment.js#o...
app/views/environment.js:235: 'span.config-file-upload': { 'click':
'_showFileDialogue' },
Where did these functions and events go? There's no removal of a function as
well.

[jeff.pihach, 2013-08-01 19:50:23]

Thanks for the reviews - comments within.

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

https://codereview.appspot.com/12205043/diff/13002/app/templates/service-conf...
app/templates/service-configuration.partial:13: id="input-{{name}}"
data-bind="{{#if ../../ghost}}options{{else}}config{{/if}}.{{name}}" {{value}}
{{#if ../../ghost}}disabled{{/if}}/>
On 2013/08/01 18:19:10, rharding wrote:
> Can you newline before the data-bind and after the value? It's already split
and
> should be safe. 

Done.

https://codereview.appspot.com/12205043/diff/13002/app/templates/service-conf...
app/templates/service-configuration.partial:25: <textarea data-bind="{{#if
../../../ghost}}options{{else}}config{{/if}}.{{name}}" class="config-field"
id="input-{{name}}" name="{{name}}" {{#if
../../../ghost}}disabled{{/if}}>{{value}}</textarea>
That's what I was thinking as well but a bit out of scope for this branch. I'll
make sure to followup later to clean this up

https://codereview.appspot.com/12205043/diff/13002/app/views/environment.js
File app/views/environment.js (left):

https://codereview.appspot.com/12205043/diff/13002/app/views/environment.js#o...
app/views/environment.js:235: 'span.config-file-upload': { 'click':
'_showFileDialogue' },
Nice catch, fns removed

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

https://codereview.appspot.com/12205043/diff/13002/app/views/ghost-inspector....
app/views/ghost-inspector.js:179: serviceConfig = container.one(
On 2013/08/01 18:19:10, rharding wrote:
> can we call this a node? container is generally a node, but serviceConfig is
> vague. 

Done.

https://codereview.appspot.com/12205043/diff/13002/app/views/ghost-inspector....
app/views/ghost-inspector.js:185: if (useDefaults) {
As discussed - the databinding engine doesn't support our usecase for this area.

https://codereview.appspot.com/12205043/diff/13002/app/views/ghost-inspector....
app/views/ghost-inspector.js:202: viewletContainer.one('#input-' +
key).set('value', newVal);
Good catch, fixed

https://codereview.appspot.com/12205043/diff/13002/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/12205043/diff/13002/app/views/utils.js#newcode...
app/views/utils.js:1437: Y.Handlebars.registerHelper('debugger', function(value)
{
Added dock comment

[jeff.pihach, 2013-08-01 20:54:30]

Please take a look.

[benjamin.saller, 2013-08-01 22:11:14]

LGTM

I had a trivial in there and you noted offline you have tests pending.

As for QA I'm very surprised there is no 'off' indicator ('x'?) on the slider.
It looks absent after you see the checkmark.

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

https://codereview.appspot.com/12205043/diff/22001/app/templates/service-conf...
app/templates/service-configuration.partial:19: {{#if
../../ghost}}disabled{{/if}}
scary scoping

https://codereview.appspot.com/12205043/diff/22001/app/templates/service-conf...
app/templates/service-configuration.partial:40: data-bind="{{#if
../../../ghost}}options{{else}}config{{/if}}.{{name}}"
and it get worse. But I understand it.

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

https://codereview.appspot.com/12205043/diff/22001/app/views/ghost-inspector....
app/views/ghost-inspector.js:200:
Y.Object.each(viewlet.model.getAttrs().options, function(opt, key) {
get('options'), not getAttrs

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

https://codereview.appspot.com/12205043/diff/22001/app/views/inspector.js#new...
app/views/inspector.js:1242: node.set('value', newVal);
One change I have pending is to put the field handler on the binding. 

In a case like this it would then become: 
    this.field.set(node, newVal)

Doesn't impact this branch, but should make update functions easier in the
future.

[jeff.pihach, 2013-08-01 22:19:54]

Thanks for the review - good idea about the X I'll add one in before landing.

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

https://codereview.appspot.com/12205043/diff/22001/app/templates/service-conf...
app/templates/service-configuration.partial:40: data-bind="{{#if
../../../ghost}}options{{else}}config{{/if}}.{{name}}"
Haha yeah this took a bit

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

https://codereview.appspot.com/12205043/diff/22001/app/views/ghost-inspector....
app/views/ghost-inspector.js:200:
Y.Object.each(viewlet.model.getAttrs().options, function(opt, key) {
good catch

[jeff.pihach, 2013-08-01 22:32:36]

*** Submitted:

Ghost default configuration toggle

This implements the 'use default configuration' toggle in the
ghost inspector. It's active by default and all inputs and 
text areas are disabled when it's active. Clicking the toggle
will re-enable all inputs. If you have changed some options
then click the toggle it will reset all values to their 
defaults.

R=j.c.sackett, rharding, benjamin.saller
CC=
https://codereview.appspot.com/12205043