Convert charm popup to full side display panel

Convert charm popup to full side display panel

This makes relatively small changes to adjust the existing charm store panels to be displayed down the full right side, per design specs.  It does not touch the display of the panels themselves, other than to adjust the scrolling.

Per reviews, some name normalization occurred as well (e.g., "panel" became "popup" in various names).

https://code.launchpad.net/~gary/juju-gui/charmpanel/+merge/131605

(do not edit description out of merge proposal)

[gary.poster, 2012-10-26 13:01:32]

Please take a look.

[thiago.veronezi, 2012-10-26 15:51:30]

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js#newco...
app/views/charm-search.js:31:
parseInt(scrollContainer.getComputedStyle('height'), 10)),
Does node.get('clientHeight') not work for this case?

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js#newco...
app/views/charm-search.js:597: svg &&
parseInt(Y.one('svg').getAttribute('height'), 10) + 17};
You dont need to call "Y.one" again. You already have the svg object.
 What happens when we have no svg object?

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js#newco...
app/views/charm-search.js:655: // if (sub) { sub.detach(); }
You don't use the sub object. Do you need the loop?

[gary.poster, 2012-10-27 09:07:10]

Thanks Thiago.  You had an excellent catch in there!  I'll fix and repropose.

Gary

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js#newco...
app/views/charm-search.js:31:
parseInt(scrollContainer.getComputedStyle('height'), 10)),
On 2012/10/26 15:51:30, thiago.veronezi wrote:
> Does node.get('clientHeight') not work for this case?

No.  They are in fact different, and I need the difference in this case.  The
height value does not include the margin or padding, and the clientHeight does. 
(Neither include the scrollbar, if any).  I need to figure out what height to
set (since clientHeight is read only) and yet I need to take the margin and
padding into account, so I need to get the difference.  Another approach would
be to add the values of margin-top, padding-top, padding-bottom, and
margin-bottom, but they are often not in pixels and it is easier this way.

I could replace the *previous* line (scrollContainer.getClientRect().height)
with offsetHeight.  Those are the same as long as the box has scrolling for
overflow.  I actually like that idea, and might do it.

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js#newco...
app/views/charm-search.js:597: svg &&
parseInt(Y.one('svg').getAttribute('height'), 10) + 17};
On 2012/10/26 15:51:30, thiago.veronezi wrote:
> You dont need to call "Y.one" again. You already have the svg object.

Correct, thanks.

>  What happens when we have no svg object?

height is undefined, which is fine for the tests.  However, your question made
me realize that this will not do what I want on interior pages!  That was
probably your point.  Thanks, good catch.  I'll fix it.

https://codereview.appspot.com/6775058/diff/1/app/views/charm-search.js#newco...
app/views/charm-search.js:655: // if (sub) { sub.detach(); }
On 2012/10/26 15:51:30, thiago.veronezi wrote:
> You don't use the sub object. Do you need the loop?

Yes.  I do use it: I call detach.  That was the intent.

[gary.poster, 2012-10-27 20:23:02]

Please take a look.

[thiago.veronezi, 2012-10-27 21:44:34]

https://codereview.appspot.com/6775058/diff/6001/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/6001/app/views/charm-search.js#ne...
app/views/charm-search.js:653: // if (sub) { sub.detach(); }
The only problem I see is that this line is commented out. :)

[gary.poster, 2012-10-28 12:32:07]

:-) Thanks Thiago

https://codereview.appspot.com/6775058/diff/6001/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/6001/app/views/charm-search.js#ne...
app/views/charm-search.js:653: // if (sub) { sub.detach(); }
On 2012/10/27 21:44:34, thiago.veronezi wrote:
> The only problem I see is that this line is commented out. :) 
LOL! Now I see what you meant.  Thank you, fixed.  :-)

[gary.poster, 2012-10-28 19:13:43]

Please take a look.

[gary.poster, 2012-10-29 16:31:58]

Please take a look.

[bac, 2012-10-30 11:12:44]

Upon grabbing your branch for testing I see there are conflicts with trunk which
include the issue of -right vs -up.

The branch looks good and behaved nicely.

Of course, wait for a Real Review.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:8: subscriptions = [],
A comment here describing the use and purpose of 'subscriptions' would be really
helpful. I figured out but only when I got to the end of this file.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:15: icon = ev.currentTarget.one('i');
Is this line really repeated?

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:18: icon.replaceClass('icon-chevron-right',
'icon-chevron-down');
-right should be -up to match the visual design. If you think the design is
wrong then the charm panel section headers need to change too.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:438: container =
Y.Node.create('<div></div>').setAttribute(
Any tag created by Y.Node.create can be self-closing and it does the right
thing.  So

Y.Node.create('<div />') 

works and is more readable, though you may argue misleading.  I see we do both. 
Just mentioning it.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:439: 'id', 'juju-search-charm-panel'),
Indent more

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:518: headerSpan.setStyle('borderLeftColor',
'lightgray');
Why isn't this styling in CSS?

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:534: if (isPopupVisible) {
These names with 'Popup' are historical and now inaccurate.  Other places you
refer to the panel (calculatePanelPosition).  The use of both is confusing.

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

https://codereview.appspot.com/6775058/diff/18001/app/views/environment.js#ne...
app/views/environment.js:1236: // "afterPageSizeRecalculation" event at the end
of this function.
Nice comment.

[gary.poster, 2012-11-01 13:39:29]

Thank you for the review Brad.  I've made all requested changes.  And intend to
land this, per the new rules.

Gary

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:8: subscriptions = [],
On 2012/10/30 11:12:44, bac wrote:
> A comment here describing the use and purpose of 'subscriptions' would be
really
> helpful. I figured out but only when I got to the end of this file.

Done.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:15: icon = ev.currentTarget.one('i');
On 2012/10/30 11:12:44, bac wrote:
> Is this line really repeated?

Heh, yes.  The duplication was in trunk (see lines 14 and 15 at left) but I did
copy it blindly.  Fixed, thank you.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:18: icon.replaceClass('icon-chevron-right',
'icon-chevron-down');
On 2012/10/30 11:12:44, bac wrote:
> -right should be -up to match the visual design. If you think the design is
> wrong then the charm panel section headers need to change too.

I believe I've changed everything as desired.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:438: container =
Y.Node.create('<div></div>').setAttribute(
On 2012/10/30 11:12:44, bac wrote:
> Any tag created by Y.Node.create can be self-closing and it does the right
> thing.  So
> 
> Y.Node.create('<div />') 
> 
> works and is more readable, though you may argue misleading.  I see we do
both. 
> Just mentioning it.

Yeah, I know.  I felt mixed about the short form.  I ended up using it.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:439: 'id', 'juju-search-charm-panel'),
On 2012/10/30 11:12:44, bac wrote:
> Indent more

Done.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:518: headerSpan.setStyle('borderLeftColor',
'lightgray');
On 2012/10/30 11:12:44, bac wrote:
> Why isn't this styling in CSS?

We talked about the explanation, but I moved it to a class.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#n...
app/views/charm-search.js:534: if (isPopupVisible) {
On 2012/10/30 11:12:44, bac wrote:
> These names with 'Popup' are historical and now inaccurate.  Other places you
> refer to the panel (calculatePanelPosition).  The use of both is confusing.

Yeah...this comment led to a lot of changes :-)  I corrected Panel -> Popup and
I also corrected many instances of search -> panel, because that is also wrong.

[gary.poster, 2012-11-01 14:44:26]

*** Submitted:

Convert charm popup to full side display panel

This makes relatively small changes to adjust the existing charm store panels to
be displayed down the full right side, per design specs.  It does not touch the
display of the panels themselves, other than to adjust the scrolling.

Per reviews, some name normalization occurred as well (e.g., "panel" became
"popup" in various names).

R=thiago.veronezi, bac
CC=
https://codereview.appspot.com/6775058