A prototype of the new ghost config

A prototype of the new ghost config

This branch implements a functional prototype of
the ghost configuration panel under the 
serviceInspector flag using the new View
Container and viewlets. There is no databinding
here because it's all local.

https://code.launchpad.net/~hatch/juju-gui/ghost-inspector/+merge/171407

(do not edit description out of merge proposal)

[jeff.pihach, 2013-06-25 20:24:41]

Please take a look.

[benjamin.saller, 2013-06-25 21:13:11]

This is a good but I would like some answers to the points raised before
approving.

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

https://codereview.appspot.com/10565043/diff/1/app/templates/ghost-config-wra...
app/templates/ghost-config-wrapper.handlebars:12: class="btn btn-primary
deploy">
Are we still using bootstrap styles here or has that changed?

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

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:43: function GhostDeployer() {}
While I appreciate this approach I think I might have used the other inspector
with some different options passed in, its very similar code and as the service
inspector grows dealing with config and constraints it will be more similar than
different.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:53: _ghostInspectors: [],
where it sounds like we must currently restrict this to one :(

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:137: id: model.get('id')
While I like keeping the viewlets lean we have a tight coupling here to the
templates in the viewlet. We are also bound up in the events. I don't think we
need to change it now, but a looser coupling would be better moving forward.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:285: // the canvas and back.
We need a better story for this :(

https://codereview.appspot.com/10565043/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10565043/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:198: this.templateConfig = options.templateConfig ||
{};
Which template is this for, all of them? Odd.

[matthew.scott, 2013-06-25 21:14:56]

The code looks okay, with a few minor comments/questions.

I'm a little concerned regarding the UX.  I'm not sure that the idea of multiple
inspectors will work with what we have in mind (notably, not having dragging be
an option, and then not having a means of switching between view containers),
and I would like to know what others think as far as this being an incremental
branch, or if work should be done right away to make the multiple-ghosts story
work.  If we are restricted to one inspector panel on the right-hand side that
changes context depending on pre- and post-deployment status, how would we work
this?

I'd like input from others before I give this a +1

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

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:68: // this is an extension `this` points to the
JujuApp
// Because this is an extension, `this`...

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:258: db.notifications.add(
Perhaps this is from old code, but do we want to expose e.err to the user?

Additionally, is there justification for not using ModelList.create()?  Just
curious.

[jeff.pihach, 2013-06-25 21:29:06]

Thanks for the reviews! Comments are below.

If we are locking the inspector to the right does that also mean that we are
limiting it to a single instance? So you cannot have multiple open at once? Or
should they lock to the right and step out from there?

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

https://codereview.appspot.com/10565043/diff/1/app/templates/ghost-config-wra...
app/templates/ghost-config-wrapper.handlebars:12: class="btn btn-primary
deploy">
This was simply copy/pasted from the original template - I expect these
templates to be blown away when we implement the new UX

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

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:43: function GhostDeployer() {}
Sorry I don't understand what you're getting at - will chat

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:53: _ghostInspectors: [],
I can restrict this to one but I think we need to open up some discussion about
a locked to the right panel and multiple ghosts

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:68: // this is an extension `this` points to the
JujuApp
On 2013/06/25 21:14:56, matthew.scott wrote:
> // Because this is an extension, `this`...

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:137: id: model.get('id')
Agree, I was torn as to where to put this, it's trivial to move it into the
viewlet so we can change with the new UX

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:258: db.notifications.add(
This was just copied over but I agree that having the 'real' error message in
the message property makes sense. WIll fix.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:285: // the canvas and back.
a-greed!

https://codereview.appspot.com/10565043/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10565043/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:198: this.templateConfig = options.templateConfig ||
{};
The templateConfig property is the config objet for the View Container hbs
template.

[bac, 2013-06-25 21:32:52]

It looks basically good but I have some questions on the behavior.  The ghost
inspector can be dismissed, which leaves a ghost hanging around indefinitely. 
It can't be destroyed (though you're given the option) because it isn't a
service.

Related, clicking on the ghost brings up the old service block menu which can
launch an inspector.  Eek.

As to the comments I made in-line I realize they are picky but I'd rather point
them out now than have confusing text in the code.  Comments are important and
should be as clear as we can make them, which is a challenge for all.

https://codereview.appspot.com/10565043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10565043/diff/1/app/app.js#newcode585
app/app.js:585: // from the Y.juju.GhostDeployer extension
I don't like this conversational style of commenting.

For the serviceInspector use the deploy method from the Y.juju.GhostDeployer
extension.

https://codereview.appspot.com/10565043/diff/1/app/app.js#newcode587
app/app.js:587: // Watch specific things, (add units), remove db.update above
Not changed, but I can't parse this sentence fragment.  Punctuation helps.

https://codereview.appspot.com/10565043/diff/1/app/app.js#newcode588
app/app.js:588: // Note: This hides under tha flag as tests don't properly clean
Less ska, ya!

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

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:38: JujuGUI app extension to add the ghost deployer
method
.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:47: A collection of the open ghost inspectors
.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:62: // XXX Is this flag still required???
I think so.  Why wouldn't it be?

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:68: // this is an extension `this` points to the
JujuApp
I don't understand this comment.  Too many this-es.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:78:
this._ghostInspectors.forEach(function(inspector, index) {
What are you doing here?  Please comment.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:108: if (typeof this.template === 'string') {
Why is this check needed?  You just defined 'template' above.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:112: // XXX not sure this should be done like this
Please sign your XXX comments so people will know who to follow up with.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:113: // but this will allow us to use the old
template
.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:175: // Check if a file has been uploaded and use
that config
.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:191: Checks the database for an existing service
with the same name
!

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:199: return (existingService) ? true : false;
return !!existingService ?

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:212: Updates the ghost service name when the user
changes it in the inspector
.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:224: a ux story for this any longer.
Maybe the real comment here and an XXX later?

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:238: a ux story for this any longer.
ditto

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:274: // Update the annotations with the box's x/y
coordinates if
(x,y)

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

https://codereview.appspot.com/10565043/diff/1/app/views/service.js#newcode1114
app/views/service.js:1114: var dd = new Y.DD.Drag({ node: container });
How about using 'var _ = new Y.DD' to better demonstrate that we don't care
about the return?

[jeff.pihach, 2013-06-25 21:56:39]

Thanks for the review - comments below, changes made and being proposed.

https://codereview.appspot.com/10565043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10565043/diff/1/app/app.js#newcode585
app/app.js:585: // from the Y.juju.GhostDeployer extension
On 2013/06/25 21:32:52, bac wrote:
> I don't like this conversational style of commenting.
> 
> For the serviceInspector use the deploy method from the Y.juju.GhostDeployer
> extension.

Done.

https://codereview.appspot.com/10565043/diff/1/app/app.js#newcode588
app/app.js:588: // Note: This hides under tha flag as tests don't properly clean
On 2013/06/25 21:32:52, bac wrote:
> Less ska, ya!

Done.

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

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:38: JujuGUI app extension to add the ghost deployer
method
On 2013/06/25 21:32:52, bac wrote:
> .

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:47: A collection of the open ghost inspectors
On 2013/06/25 21:32:52, bac wrote:
> .

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:62: // XXX Is this flag still required???
Because it comes in pre-loaded via the modelController promises - I'll elaborate
the comment and add my name

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:68: // this is an extension `this` points to the
JujuApp
the context to which this executes under is the JujuApp instance because it's an
extension. I'll update the docs

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:78:
this._ghostInspectors.forEach(function(inspector, index) {
This loops through the instantiated ghostInspectors to find one which matches
the one which has fired the destroy event notifying that it's been destroyed so
we can remove it from the collection - comment added

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:108: if (typeof this.template === 'string') {
Good catch - this is left over from the default.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:112: // XXX not sure this should be done like this
On 2013/06/25 21:32:52, bac wrote:
> Please sign your XXX comments so people will know who to follow up with.

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:113: // but this will allow us to use the old
template
On 2013/06/25 21:32:52, bac wrote:
> .

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:175: // Check if a file has been uploaded and use
that config
On 2013/06/25 21:32:52, bac wrote:
> .

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:191: Checks the database for an existing service
with the same name
On 2013/06/25 21:32:52, bac wrote:
> !

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:199: return (existingService) ? true : false;
Implementers Choice :-P

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:212: Updates the ghost service name when the user
changes it in the inspector
On 2013/06/25 21:32:52, bac wrote:
> .

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:224: a ux story for this any longer.
On 2013/06/25 21:32:52, bac wrote:
> Maybe the real comment here and an XXX later?

Done.

https://codereview.appspot.com/10565043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:238: a ux story for this any longer.
On 2013/06/25 21:32:52, bac wrote:
> ditto

Done.

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

https://codereview.appspot.com/10565043/diff/1/app/views/service.js#newcode1114
app/views/service.js:1114: var dd = new Y.DD.Drag({ node: container });
This will not be draggable in a follow-up and will be removed.

[jeff.pihach, 2013-06-25 21:58:55]

Please take a look.

[matthew.scott, 2013-06-25 22:07:28]

LGTM - thanks for moving this forward!

[jeff.pihach, 2013-06-26 17:28:45]

Please take a look.

[benjamin.saller, 2013-06-26 17:49:16]

LGTM

Thanks for keeping with this. Less duplicated code moving forward is going to be
a win.

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

https://codereview.appspot.com/10565043/diff/20001/app/views/ghost-inspector....
app/views/ghost-inspector.js:115: Y.namespace('juju.controller').ghostInspector
= {
Less is more :)

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

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1128: var prototype = {};
I think this name is too generic. Module context is fine, but I'd still like to
qualify this or assign directly under the object.

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1153: });
This doesn't allow for overriding aspects of the config which the deep merge
did. Is there a reason you like this better? Would the older format be more
clear if we put a wrapper method around Y.Mix such that the arguments were
hidden?

options.viewlets = deepMerge(DEFAULT_VIEWLETS,       options.viewlets)

for example.

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1178: // that the will change
'that the model will change'

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1181: this.inspector.showViewlet(options.viewletList[0]);
maybe default: true on the viewlet or viewlets.default = 'name'. I'm also fine
with [0] but we should document that behavior.

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

https://codereview.appspot.com/10565043/diff/20001/app/views/topology/service...
app/views/topology/service.js:1338: viewletList: ['overview', 'units',
'config'],
We should update the template to use this list when generating tabs but that can
come later.

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

Thanks for the review!

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

https://codereview.appspot.com/10565043/diff/20001/app/views/ghost-inspector....
app/views/ghost-inspector.js:115: Y.namespace('juju.controller').ghostInspector
= {
:)

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

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1128: var prototype = {};
I'll be adding documentation and I can make the property name more specific

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1153: });
I was under the impression that the viewlets were static now but I can change
that back and throw a comment above the mix to say what's going on.

https://codereview.appspot.com/10565043/diff/20001/app/views/service.js#newco...
app/views/service.js:1178: // that the will change
On 2013/06/26 17:49:17, benjamin.saller wrote:
> 'that the model will change'

Done.

[jeff.pihach, 2013-06-26 19:00:34]

Please take a look.

[jeff.pihach, 2013-06-26 19:17:07]

*** Submitted:

A prototype of the new ghost config

This branch implements a functional prototype of
the ghost configuration panel under the 
serviceInspector flag using the new View
Container and viewlets. There is no databinding
here because it's all local.

R=matthew.scott, bac, benjamin.saller
CC=
https://codereview.appspot.com/10565043