Left inspector panel charm details styling

Left inspector panel charm details styling

This is Huw's branch: https://codereview.appspot.com/11999043/ with
a minor refactoring of the viewlet code to simplify the rendering
of viewlets rendered into hidden slots.

The issues still outstanding are:
Tab name appears as content while loading real content
Loading indicator appears over the tab labels while loading content

https://code.launchpad.net/~hatch/juju-gui/panel-styling-huw/+merge/177458

(do not edit description out of merge proposal)

[jeff.pihach, 2013-07-29 19:46:14]

Please take a look.

[benjamin.saller, 2013-07-29 20:15:41]

Close, done without QA, I can do that after the changes mentioned are talked
about/fixed. 

Thanks

https://codereview.appspot.com/12054043/diff/1/app/templates/left-breakout-pa...
File app/templates/left-breakout-panel.handlebars (right):

https://codereview.appspot.com/12054043/diff/1/app/templates/left-breakout-pa...
app/templates/left-breakout-panel.handlebars:2: <a href="" class="close-slot"
data-slot="left-hand-panel"></a>
You're nesting the slot inside an <a>? I think div is still what you want here.
Am I missing a reason?

The change to data-slot is nice though.

https://codereview.appspot.com/12054043/diff/1/app/templates/service-inspecto...
File app/templates/service-inspector.handlebars (right):

https://codereview.appspot.com/12054043/diff/1/app/templates/service-inspecto...
app/templates/service-inspector.handlebars:3: <div class="left-breakout
yui3-skin-sam" style="display: none;"></div>
I thought we were going to drop the display: none here?

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

https://codereview.appspot.com/12054043/diff/1/app/views/viewlet-manager.js#n...
app/views/viewlet-manager.js:368: // Shows the slot
Can we move this into fillSlot and reuse the query result done there? Its
unconditional so we should keep as much slot logic there as possible.

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

https://codereview.appspot.com/12054043/diff/1/test/test_viewlet_manager.js#n...
test/test_viewlet_manager.js:262: it.skip('can remove a slot from the dom',
function() {
The answer here is to fix this then, not skip it. This is a test of the
viewlet-manager and the ability to replace a slot is something we depend on.

[jeff.pihach, 2013-07-29 20:20:18]

Thanks for the review - replies below.

https://codereview.appspot.com/12054043/diff/1/app/templates/left-breakout-pa...
File app/templates/left-breakout-panel.handlebars (right):

https://codereview.appspot.com/12054043/diff/1/app/templates/left-breakout-pa...
app/templates/left-breakout-panel.handlebars:2: <a href="" class="close-slot"
data-slot="left-hand-panel"></a>
The data-slot is simply an indicator to tell the viewlet-manager which slot to
close when this button is clicked. The viewlet still gets rendered into the
content section.

https://codereview.appspot.com/12054043/diff/1/app/templates/service-inspecto...
File app/templates/service-inspector.handlebars (right):

https://codereview.appspot.com/12054043/diff/1/app/templates/service-inspecto...
app/templates/service-inspector.handlebars:3: <div class="left-breakout
yui3-skin-sam" style="display: none;"></div>
Because this slot is not visible by default, to avoid a flash of this viewlet
wrapper styles, it needs to be hidden as soon as it's rendered.

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

https://codereview.appspot.com/12054043/diff/1/app/views/viewlet-manager.js#n...
app/views/viewlet-manager.js:368: // Shows the slot
I was under the impression from our discussion that this was to be here because
fillSlot doesn't indicate that it's also showing it.

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

https://codereview.appspot.com/12054043/diff/1/test/test_viewlet_manager.js#n...
test/test_viewlet_manager.js:262: it.skip('can remove a slot from the dom',
function() {
True, I missed this when updating Huw's branch, I'll fix this tests.

[benjamin.saller, 2013-07-29 20:44:20]

Thanks for the followup, let me know when you want another review.

https://codereview.appspot.com/12054043/diff/1/app/templates/left-breakout-pa...
File app/templates/left-breakout-panel.handlebars (right):

https://codereview.appspot.com/12054043/diff/1/app/templates/left-breakout-pa...
app/templates/left-breakout-panel.handlebars:2: <a href="" class="close-slot"
data-slot="left-hand-panel"></a>
On 2013/07/29 20:20:18, jeff.pihach wrote:
> The data-slot is simply an indicator to tell the viewlet-manager which slot to
> close when this button is clicked. The viewlet still gets rendered into the
> content section.

Ahh, I thought I'd just missed the branch where we reverse the slots mapping in
the manager from the DOM. We might want to do that at some point.

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

https://codereview.appspot.com/12054043/diff/1/app/views/viewlet-manager.js#n...
app/views/viewlet-manager.js:368: // Shows the slot
On 2013/07/29 20:20:18, jeff.pihach wrote:
> I was under the impression from our discussion that this was to be here
because
> fillSlot doesn't indicate that it's also showing it. 

I suppose that is true. I saw fillSlot as an impl detail hidden under
showViewlet, but maybe they need to be used independently. However I don't think
we have anything that does this today and I still think its a better
encapsulation to put as much slot code as possible in a few dedicated methods.

[jeff.pihach, 2013-07-29 20:46:35]

Please take a look.

[benjamin.saller, 2013-07-29 20:52:21]

LGTM, thanks

I think the slots def change coming from data-slot and not defining the mapping
in code would be a win down the road.

[jeff.pihach, 2013-07-29 21:14:41]

*** Submitted:

Left inspector panel charm details styling

This is Huw's branch: https://codereview.appspot.com/11999043/ with
a minor refactoring of the viewlet code to simplify the rendering
of viewlets rendered into hidden slots.

The issues still outstanding are:
Tab name appears as content while loading real content
Loading indicator appears over the tab labels while loading content

R=benjamin.saller
CC=
https://codereview.appspot.com/12054043