Code review comment for lp://staging/~gary/juju-gui/ghostDeploy

Revision history for this message
Gary Poster (gary) wrote :

95% fly-bys. Sorry.

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js#newcode859
app/store/env/sandbox.js:859: return null; // Probably unit/service was
deleted.
This was a flyby: when deleting a service then sometimes this would
trigger. I'll remove if a test is needed; it is just for exploratory
testing AFAIK.

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js#newcode868
app/store/env/sandbox.js:868: return null; // Probably unit/service was
deleted.
As above.

https://codereview.appspot.com/13246050/diff/1/app/templates/service-constraints-viewlet.handlebars
File app/templates/service-constraints-viewlet.handlebars (right):

https://codereview.appspot.com/13246050/diff/1/app/templates/service-constraints-viewlet.handlebars#newcode3
app/templates/service-constraints-viewlet.handlebars:3: <h2>Constraints
for new units</h2>
Fly-by. Every other title is sentence cased. I prefer title cased, but
this is easier for now, and I'm not sure if this is actually something
that UX cares about.

https://codereview.appspot.com/13246050/diff/1/app/templates/service-relations-viewlet.handlebars
File app/templates/service-relations-viewlet.handlebars (right):

https://codereview.appspot.com/13246050/diff/1/app/templates/service-relations-viewlet.handlebars#newcode2
app/templates/service-relations-viewlet.handlebars:2: <h2>Relations</h2>
Every other non-intro tab has a header. It needs one. Sorry, another
fly-by.

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

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode306
app/views/ghost-inspector.js:306: ghostService.set('localCreation',
true);
This line is about...well, see the comment.

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode307
app/views/ghost-inspector.js:307:
this.options.environment.createServiceInspector(ghostService);
This was the only change that this branch actually needed to accomplish
the stated task. Sorry.

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

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js#newcode1391
app/views/topology/service.js:1391:
serviceMenu.one('.destroy-service').hide();
Another fly-by. We should actually make this menu show and hide with
inspector, but I was amazingly able to hold myself back from
implementing that. (It was because I couldn't see how to do it quickly
:-P).

https://codereview.appspot.com/13246050/diff/1/app/views/viewlets/inspector-header.js
File app/views/viewlets/inspector-header.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/viewlets/inspector-header.js#newcode35
app/views/viewlets/inspector-header.js:35: if (model instanceof
models.BrowserCharm) {
Another flyby. Jeff pointed out the .scheme conditional, which seemed
unnecessarily obscure to me. This seems nice and clear to me.

https://codereview.appspot.com/13246050/

« Back to merge proposal