Merge lp://staging/~gary/juju-gui/charmpanel into lp://staging/juju-gui/experimental

Reviewers: mp+131605_code.launchpad.net,

Message:
Please take a look.

Description:
Convert charm store to full-side display

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 pnales themselves, other than to
adjust the scrolling.

Thanks

Gary

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6775058/

Affected files:
   A [revision details]
   D app/templates/charm-search-pop.handlebars
   M app/views/charm-search.js
   M app/views/environment.js
   M lib/views/stylesheet.less

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#newcode31
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#newcode597
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#newcode655
app/views/charm-search.js:655: // if (sub) { sub.detach(); }
You don't use the sub object. Do you need the loop?

https://codereview.appspot.com/6775058/

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#newcode31
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#newcode597
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#newcode655
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.

https://codereview.appspot.com/6775058/

Please take a look.

https://codereview.appspot.com/6775058/

Note that the charm panel does not have a defined good behavior on the two pages with legacy designs: service units and charms. The former will be redone for the new visual design, and the latter will be deleted.

Also, I think this fixes the occasional mis-sizing of the environment on window resize.

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#newcode653
app/views/charm-search.js:653: // if (sub) { sub.detach(); }
The only problem I see is that this line is commented out. :)

https://codereview.appspot.com/6775058/

:-) 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#newcode653
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. :-)

https://codereview.appspot.com/6775058/

Please take a look.

https://codereview.appspot.com/6775058/

Please take a look.

https://codereview.appspot.com/6775058/

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#newcode8
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#newcode15
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#newcode18
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#newcode438
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#newcode439
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#newcode518
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#newcode534
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#newcode1236
app/views/environment.js:1236: // "afterPageSizeRecalculation" event at
the end of this function.
Nice comment.

https://codereview.appspot.com/6775058/

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#newcode8
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#newcode15
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#newcode18
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#newcode438
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#newcode439
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#newcode518
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#newcode534
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.

https://codereview.appspot.com/6775058/

*** 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

https://codereview.appspot.com/6775058/