Inspector settings page with fileupload

Inspector settings page with fileupload

This branch makes the service inspector settings panel
closely resemble the wireframe and adds in the UI/UX
for the config file upload button. The button doesn't
actually send the configuration values anywhere but the
file is read into a property on the view container.

https://code.launchpad.net/~hatch/juju-gui/inspector-settings/+merge/172213

(do not edit description out of merge proposal)

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

Please take a look.

[bac, 2013-07-02 14:46:41]

The code that is here looks pretty good except for the few things I've cited,
like the missing service name.  We need tests for this part of the inspector,
especially the interaction with file upload.

It is unclear to me if changes are actually taking affect.  Does the
data-binding work cause changed values to be stored when the panel is closed?

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:2: <h2>{{service.displayName}}
settings</h2>
{{service.displayName}} is not being rendered.

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
File app/templates/service-configuration.partial (right):

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
app/templates/service-configuration.partial:10: <label
for="input-{{name}}">{{name}}</label>
Nice

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

https://codereview.appspot.com/10690046/diff/1/app/views/environment.js#newco...
app/views/environment.js:205: 'input[name=service-name]': { valuechange:
'updateGhostName' }
Why can you get away with not quoting valuechange?

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

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1055
app/views/service.js:1055: Handles showing/hiding the configuration settings
descriptions
full-stop

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1076
app/views/service.js:1076: depending if a file has beem previously loaded or not
typo: been
full-stop

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1091
app/views/service.js:1091: parse the file data
full-stop

https://codereview.appspot.com/10690046/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (left):

https://codereview.appspot.com/10690046/diff/1/lib/views/stylesheet.less#oldc...
lib/views/stylesheet.less:204: }
Why was this here?  Nice simplification.

[benji, 2013-07-02 16:48:42]

Good stuff.

LGTM-ly y'rs,
Benji

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
File app/templates/service-configuration.partial (right):

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
app/templates/service-configuration.partial:10: <label
for="input-{{name}}">{{name}}</label>
On 2013/07/02 14:46:42, bac wrote:
> Nice

Indeed.

https://codereview.appspot.com/10690046/diff/1/app/templates/view-container.h...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10690046/diff/1/app/templates/view-container.h...
app/templates/view-container.handlebars:3: <div class="close" style="position:
absolute; right: 0; border: 1px solid black; padding: 3px;">X</div>
Is moving this to a style sheet going to be part of the follow-on branch?

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

https://codereview.appspot.com/10690046/diff/1/app/views/environment.js#newco...
app/views/environment.js:205: 'input[name=service-name]': { valuechange:
'updateGhostName' }
On 2013/07/02 14:46:42, bac wrote:
> Why can you get away with not quoting valuechange?

We don't have to quote it because it is a valid JS identifier.  Consistency one
way or the other would be nice here though.

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

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1065
app/views/service.js:1065: if (e.currentTarget.getHTML().indexOf('Hide') < 0) {
I had to read this twice before realizing what the code meant.  Making this ">
0" and swapping the if and else code blocks would make it clearer.  Right now
the negative branch is first, which is slightly harder to understand.

I don't feel strongly about this part, but I want to see if it sparks any ideas
for you: I wonder if interrogating the HTML is the best way to do this.  It
seems that we should have some internal state that we depend on instead of
deciding on what to do based on reading back the UI that we generated earlier.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1082
app/views/service.js:1082: if (e.currentTarget.getHTML().indexOf('Remove') < 0)
{
As above, putting the positive branch first would be better.

Right now this reads as:

If "Remove" isn't on the screen, let the user choose a file to upload, otherwise
remove the previously selected file.

It would be better the other way:

If "Remove" is on the screen, remove the previously selected file, otherwise let
the user choose a file to upload.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1083
app/views/service.js:1083:
e.container.one('input[type=file]').getDOMNode().click();
I think I understand what this does, but a comment like "Open the file browser
so the user can select a configuration file." would be nice.

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

https://codereview.appspot.com/10690046/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:109: render: function(model, viewContainerAttrs) {
Presumably the parameter wasn't listed because it isn't used.

If it stays it should be documented.

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

https://codereview.appspot.com/10690046/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:1: 
I suspect these newlines were unintentional.

[jeff.pihach, 2013-07-02 17:26:01]

Thanks greatly for the reviews guys - updated version coming soon

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/10690046/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:2: <h2>{{service.displayName}}
settings</h2>
as per irc changed to read Service Settings

https://codereview.appspot.com/10690046/diff/1/app/templates/view-container.h...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10690046/diff/1/app/templates/view-container.h...
app/templates/view-container.handlebars:3: <div class="close" style="position:
absolute; right: 0; border: 1px solid black; padding: 3px;">X</div>
I figured this would be cleaned up once we have some more assets to be able to
do the inspector wrapper styling properly.

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

https://codereview.appspot.com/10690046/diff/1/app/views/environment.js#newco...
app/views/environment.js:205: 'input[name=service-name]': { valuechange:
'updateGhostName' }
None of these have to be quoted - I removed the quotes because I needed to add a
, on the end for an event (which is now removed) while keeping under the 80char
limit :)

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

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1055
app/views/service.js:1055: Handles showing/hiding the configuration settings
descriptions
On 2013/07/02 14:46:42, bac wrote:
> full-stop

Done.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1065
app/views/service.js:1065: if (e.currentTarget.getHTML().indexOf('Hide') < 0) {
it was my intention to keep as much state in the DOM as possible to keep these
viewlet configs as stateless as possible - if this turns out to be a bad idea we
can certainly revisit it in the future.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1076
app/views/service.js:1076: depending if a file has beem previously loaded or not
On 2013/07/02 14:46:42, bac wrote:
> typo: been
> full-stop

Done.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1083
app/views/service.js:1083:
e.container.one('input[type=file]').getDOMNode().click();
On 2013/07/02 16:48:42, benji wrote:
> I think I understand what this does, but a comment like "Open the file browser
> so the user can select a configuration file." would be nice.

Done.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1091
app/views/service.js:1091: parse the file data
On 2013/07/02 14:46:42, bac wrote:
> full-stop

Done.

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

https://codereview.appspot.com/10690046/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:109: render: function(model, viewContainerAttrs) {
On 2013/07/02 16:48:42, benji wrote:
> Presumably the parameter wasn't listed because it isn't used.
> 
> If it stays it should be documented.

Done.

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

https://codereview.appspot.com/10690046/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:1: 
On 2013/07/02 16:48:42, benji wrote:
> I suspect these newlines were unintentional.

Done.

[jeff.pihach, 2013-07-02 17:50:50]

Please take a look.

[bac, 2013-07-02 18:06:19]

On 2013/07/02 17:50:50, jeff.pihach wrote:
> Please take a look.

LGTM as an untested, UI-only branch.  Thanks.

[bac, 2013-07-02 18:06:44]

.

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

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1065
app/views/service.js:1065: if (e.currentTarget.getHTML().indexOf('Hide') < 0) {
Benji's first point was the if condition would read better if it was
affirmatively testing for 'Hide' and performing that action in the 'then' part. 
Seems like a good idea to me, especially if you use '>= 0'.

https://codereview.appspot.com/10690046/diff/1/app/views/service.js#newcode1082
app/views/service.js:1082: if (e.currentTarget.getHTML().indexOf('Remove') < 0)
{
Ditto

[jeff.pihach, 2013-07-02 18:47:43]

*** Submitted:

Inspector settings page with fileupload

This branch makes the service inspector settings panel
closely resemble the wireframe and adds in the UI/UX
for the config file upload button. The button doesn't
actually send the configuration values anywhere but the
file is read into a property on the view container.

R=bac, benji
CC=
https://codereview.appspot.com/10690046