Break out left breakout panel from juju-inspector

Break out left breakout panel from juju-inspector

- In order for items to be reused as a left-breakout they cannot be under the
.juju-inspector css tree or else it becomes a css nightmare.
- This restructures the default html layout so that the yui3-inspector-panel
is css agnostic and inside of it are the two main components, .juju-inspector
and the left breakout panel.
- The CharmDetails uses the left breakout panel and is situated to show inside
of that.
- Due to these changes some css needed tweaking.
- Anything that wants to reuse the .juju-inspector css rules in the left
breakout panel will have to add that css class to their own template output.


QA:

To QA, run the gui with the serviceInspector flag and deploy a service. Then
click on that service to bring up the inspector and click on the cs:... url to
open the charm details view into the left breakout panel.

https://code.launchpad.net/~rharding/juju-gui/charm-details-viewlet-css/+merge/176354

(do not edit description out of merge proposal)

[rharding, 2013-07-23 12:16:24]

Please take a look.

[bac, 2013-07-23 12:26:01]

Code LGTM but I haven't QA'd.  Thanks for this wonderful cleanup and re-org.

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

https://codereview.appspot.com/11714043/diff/1/app/views/inspector.js#newcode...
app/views/inspector.js:1254: container.appendTo(Y.one('#content'));
Very nice.

[benji, 2013-07-23 12:55:24]

Good stuff.  I didn't see any show stoppers.  I noted a few small things you
might want to look at.

LGTM-and-I-wonder-how-many-people-get-this-reference-ly, y'rs
Benji

https://codereview.appspot.com/11714043/diff/1/app/views/viewlet-manager.js
File app/views/viewlet-manager.js (right):

https://codereview.appspot.com/11714043/diff/1/app/views/viewlet-manager.js#n...
app/views/viewlet-manager.js:145: 
@return?

https://codereview.appspot.com/11714043/diff/1/app/views/viewlets/charm-detai...
File app/views/viewlets/charm-details.js (right):

https://codereview.appspot.com/11714043/diff/1/app/views/viewlets/charm-detai...
app/views/viewlets/charm-details.js:36: 
@return

Also, the blank line before the comment close is a new one on me.

https://codereview.appspot.com/11714043/diff/1/lib/views/browser/charm-full.less
File lib/views/browser/charm-full.less (right):

https://codereview.appspot.com/11714043/diff/1/lib/views/browser/charm-full.l...
lib/views/browser/charm-full.less:2: .charm {
Isn't "#subapp-browser .charm" a subset of ".charm" and therefore unnecessary?

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

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:46: margin: 0; /* Can be removed once the old
inspector styles are removed. */
It would be nice if we could avoid this long line.

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:79: padding: 0 20px;*/
Commented-out code.

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:153: background: transparent
url(/juju-ui/assets/images/charm_64.png) left top no-repeat;
It would be nice if we could avoid this long line (that, admittedly, was already
there).

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:217: hyphenate-lines: 3;
The above would make a great less function (or whatever they call it).

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:367: /* Make sure the destroy icon is below the
close-the-inspector icon. */
It would be nice if we could avoid this long line.

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:396: box-shadow: 0 2px 2px -2px #777777 inset;
Do we need the "box-shadow" line if we have the ".create-box-shadow" line?

https://codereview.appspot.com/11714043/diff/1/test/test_viewlet_manager.js
File test/test_viewlet_manager.js (right):

https://codereview.appspot.com/11714043/diff/1/test/test_viewlet_manager.js#n...
test/test_viewlet_manager.js:33: '</div>'].join(''));
Generally, the project doesn't have much HTML-in-strings like the above. 
Perhaps a template or Y.Node calls instead:

container
    .append(Y.Node.create('<div/>')
        .addClass('yui3-juju-inspector')
        .append(Y.Node.create('<div/>')
            .addClass('panel')
            .addClass('juju-inspector'))
        .append(Y.Node.create('<div/>')
            .addClass('left-breakout')))

[rharding, 2013-07-23 13:11:26]

Comments below. New lbox of the branch on the way.

https://codereview.appspot.com/11714043/diff/1/app/views/viewlet-manager.js
File app/views/viewlet-manager.js (right):

https://codereview.appspot.com/11714043/diff/1/app/views/viewlet-manager.js#n...
app/views/viewlet-manager.js:145: 
On 2013/07/23 12:55:24, benji wrote:
> @return?

In talking with Jeff about this if there is no return value then the concensus
seems to be to leave it out? I've seen something like 

@returns {undefined} Nothing.

Yet that seems like just line noise imo. I can add if it's the standard though.

https://codereview.appspot.com/11714043/diff/1/app/views/viewlets/charm-detai...
File app/views/viewlets/charm-details.js (right):

https://codereview.appspot.com/11714043/diff/1/app/views/viewlets/charm-detai...
app/views/viewlets/charm-details.js:36: 
On 2013/07/23 12:55:24, benji wrote:
> @return
> 
> Also, the blank line before the comment close is a new one on me.

Heh, this is my inner PEP257 coming out. I find it easier to read. I'll remove.

https://codereview.appspot.com/11714043/diff/1/lib/views/browser/charm-full.less
File lib/views/browser/charm-full.less (right):

https://codereview.appspot.com/11714043/diff/1/lib/views/browser/charm-full.l...
lib/views/browser/charm-full.less:2: .charm {
On 2013/07/23 12:55:24, benji wrote:
> Isn't "#subapp-browser .charm" a subset of ".charm" and therefore unnecessary?

There are later bits of css that include the #subapp-browser as part of a chain
of CSS. 

#subapp-browser .charm .one .two h1 

In this case, it's considered 'more specific' and the css rule will not apply if
it's just .charm .one .two h1. So I had to make sure both applied to help
prevent breakage of current css.

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

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:46: margin: 0; /* Can be removed once the old
inspector styles are removed. */
On 2013/07/23 12:55:24, benji wrote:
> It would be nice if we could avoid this long line.

Updated.

https://codereview.appspot.com/11714043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:79: padding: 0 20px;*/
On 2013/07/23 12:55:24, benji wrote:
> Commented-out code.

This is old code indented under the new .juju-inspector space. I didnt' audit it
for other issues. I'll remove.

https://codereview.appspot.com/11714043/diff/1/test/test_viewlet_manager.js
File test/test_viewlet_manager.js (right):

https://codereview.appspot.com/11714043/diff/1/test/test_viewlet_manager.js#n...
test/test_viewlet_manager.js:33: '</div>'].join(''));
On 2013/07/23 12:55:24, benji wrote:
> Generally, the project doesn't have much HTML-in-strings like the above. 
> Perhaps a template or Y.Node calls instead:
> 
> container
>     .append(Y.Node.create('<div/>')
>         .addClass('yui3-juju-inspector')
>         .append(Y.Node.create('<div/>')
>             .addClass('panel')
>             .addClass('juju-inspector'))
>         .append(Y.Node.create('<div/>')
>             .addClass('left-breakout')))

Hmm, personally I find the second much harder to read what html is generated and
it's a 7-1 function call performance hit. This function is reused so I'd hate to
go looping that. Is there something that's un-preferred about the join?

[rharding, 2013-07-23 13:16:23]

Please take a look.

[rharding, 2013-07-23 14:19:21]

*** Submitted:

Break out left breakout panel from juju-inspector

- In order for items to be reused as a left-breakout they cannot be under the
.juju-inspector css tree or else it becomes a css nightmare.
- This restructures the default html layout so that the yui3-inspector-panel
is css agnostic and inside of it are the two main components, .juju-inspector
and the left breakout panel.
- The CharmDetails uses the left breakout panel and is situated to show inside
of that.
- Due to these changes some css needed tweaking.
- Anything that wants to reuse the .juju-inspector css rules in the left
breakout panel will have to add that css class to their own template output.


QA:

To QA, run the gui with the serviceInspector flag and deploy a service. Then
click on that service to bring up the inspector and click on the cs:... url to
open the charm details view into the left breakout panel.

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