Add destroy service and destroy ghost facility.

Add destroy service and destroy ghost facility.



https://code.launchpad.net/~benji/juju-gui/destroy-controls/+merge/174232

(do not edit description out of merge proposal)

[benji, 2013-07-11 16:03:21]

Please take a look.

[jeff.pihach, 2013-07-11 16:37:51]

Code looks good, I like that we can just use css transitions too.

The QA failed because the destroy button doesn't do anything in the ghost
config.

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

https://codereview.appspot.com/11169043/diff/1/app/views/service.js#newcode1161
app/views/service.js:1161: var dataSource = svcInspector ? this.inspector :
this;
could you add a comment that the new instance is `this.inspector` when the flags
are gone.

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

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:214: float: right;
should add

cursor: pointer;

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:233: -webkit-transition: 1s, height 1s;
Glad you were able to go with CSS instead of the transition module

[matthew.scott, 2013-07-11 16:42:33]

LGTM-ly with destroy working on ghosts.

https://codereview.appspot.com/11169043/diff/1/app/templates/ghost-config-wra...
File app/templates/ghost-config-wrapper.handlebars (right):

https://codereview.appspot.com/11169043/diff/1/app/templates/ghost-config-wra...
app/templates/ghost-config-wrapper.handlebars:4: {{>service-destroy}}
This shows up, but doesn't do anything when I click on it

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

https://codereview.appspot.com/11169043/diff/1/app/views/service.js#newcode1198
app/views/service.js:1198: // service from the database.  (Why wouldn't we get
an update from the
This would come later in a delta, right?  We should be getting a ['service',
'remove', {...}] delta from pyjuju, at least.  Perhaps we have been just acting
preemptively by removing on a non-error in the callback.

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

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:244: -webkit-border-radius: 4px;
.create-border-radius(4px);

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:247: -webkit-box-shadow: 0 2px 2px -2px #777777
inset;
.create-box-shadow(0 2px 2px -2px #777 inset);

https://codereview.appspot.com/11169043/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/11169043/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:115: /**** Begin service destroy UI tests. ****/
Can this maybe be an inner describe()?  That will keep all previous variables
and functions in scope and will also show in the output.

[benji, 2013-07-11 20:51:23]

All review comments addressed, including the bug that caused the icon click to
be ignored on ghost inspectors.

https://codereview.appspot.com/11169043/diff/1/app/templates/ghost-config-wra...
File app/templates/ghost-config-wrapper.handlebars (right):

https://codereview.appspot.com/11169043/diff/1/app/templates/ghost-config-wra...
app/templates/ghost-config-wrapper.handlebars:4: {{>service-destroy}}
On 2013/07/11 16:42:33, matthew.scott wrote:
> This shows up, but doesn't do anything when I click on it

I apparently munged the class name when merging in trunk.  I added a test that
would have caught this and fixed it.

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

https://codereview.appspot.com/11169043/diff/1/app/views/service.js#newcode1161
app/views/service.js:1161: var dataSource = svcInspector ? this.inspector :
this;
On 2013/07/11 16:37:51, jeff.pihach wrote:
> could you add a comment that the new instance is `this.inspector` when the
flags
> are gone.

Done.

https://codereview.appspot.com/11169043/diff/1/app/views/service.js#newcode1198
app/views/service.js:1198: // service from the database.  (Why wouldn't we get
an update from the
On 2013/07/11 16:42:33, matthew.scott wrote:
> This would come later in a delta, right?  We should be getting a ['service',
> 'remove', {...}] delta from pyjuju, at least.  Perhaps we have been just
acting
> preemptively by removing on a non-error in the callback.

I would have thought the same thing, except that when this code is removed the
now-dead service lingers (apparently) forever in the GUI.

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

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:214: float: right;
On 2013/07/11 16:37:51, jeff.pihach wrote:
> should add
> 
> cursor: pointer;

Ah, good one.  Fixed.

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:233: -webkit-transition: 1s, height 1s;
On 2013/07/11 16:37:51, jeff.pihach wrote:
> Glad you were able to go with CSS instead of the transition module

Yeah, it turned out really well.

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:244: -webkit-border-radius: 4px;
On 2013/07/11 16:42:33, matthew.scott wrote:
> .create-border-radius(4px);

Ooh!  Very nice.

https://codereview.appspot.com/11169043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:247: -webkit-box-shadow: 0 2px 2px -2px #777777
inset;
On 2013/07/11 16:42:33, matthew.scott wrote:
> .create-box-shadow(0 2px 2px -2px #777 inset);

Nice.

https://codereview.appspot.com/11169043/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/11169043/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:115: /**** Begin service destroy UI tests. ****/
On 2013/07/11 16:42:33, matthew.scott wrote:
> Can this maybe be an inner describe()?  That will keep all previous variables
> and functions in scope and will also show in the output.

I had similar thoughts, but the additional indentation really turns me off. 
Since there is only one other test in this group, I'll leave it as-is for now,
but we should break these apart if we add more in the future.

[benji, 2013-07-11 21:08:47]

Please take a look.

[jeff.pihach, 2013-07-11 21:12:50]

LGTM Thanks for those fixes, 

QA Good but I think you applied the pointer:cursor to the wrong element

[benji, 2013-07-11 21:44:02]

*** Submitted:

Add destroy service and destroy ghost facility.

R=jeff.pihach, matthew.scott
CC=
https://codereview.appspot.com/11169043