Adds charm container widget.

Adds charm container widget.

-Adds a new widget, CharmContainer. CharmContainer accepts CharmSmall widgets
as children. It has a set cutoff which is the maximum number of items it will
intially show, and an expander action that shows/hides the remaining items. It
can be named, and is meant to be used to loosely categorize a group of charms,
e.g. "Popular", "New".

https://code.launchpad.net/~jcsackett/juju-gui/charm-container/+merge/155955

(do not edit description out of merge proposal)

[j.c.sackett, 2013-03-28 13:04:24]

Please take a look.

[jeff.pihach, 2013-03-28 15:20:09]

Thanks for this! I have just a few minor improvements listed below.

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js
File app/widgets/charm-container.js (right):

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:23: Y.WidgetParent
Just an FYI the widget parent/child stuff hasn't been worked on in a very long
time and is likely due for a complete refactoring

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:37: total = this._items.length,
widget parent has a size() method
http://yuilibrary.com/yui/docs/api/classes/WidgetParent.html#method_size

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:49: var cut_items =
this._items.slice(this.get('cutoff'), this.get('total'));
cutItems ;)

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:79: var msg = Y.Lang.sub('See {extra} more',
{extra: this.get('extra')});
In charm-container.handlebars you have this identical string. Possible bug?

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:94: this._events.push(expander.on('click',
this._toggleExpand, this));
If you use a property and not a attribute here then you will share events across
all instances of this module.

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:137: all_visible: {
allVisible :)

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

https://codereview.appspot.com/8080043/diff/1/lib/views/browser/charm-small.l...
lib/views/browser/charm-small.less:6: div#main:hover button.hidden {
I don't think having a nested ID a very good idea - then you can only ever have
one instance of yui3-charmsmall.

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js
File test/test_charm_container.js (right):

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js#ne...
test/test_charm_container.js:23: container.remove(true);
remove(true) doesn't call destroy(true) (unfortunately) so a more thorough way
to destroy this would be `container.remove().destroy(true)` - I just found this
out yesterday. :-D

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js#ne...
test/test_charm_container.js:25: charm_container.destroy();
destroy(true) to remove all event listeners from children as well.

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js#ne...
test/test_charm_container.js:30: charm_container = new
Y.juju.widgets.browser.CharmContainer({
the instances of CharmContainer() are never being destroyed.

[j.c.sackett, 2013-03-28 16:34:19]

Thanks for comments; I've made most of those fixes, but have some questions
about others.

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js
File app/widgets/charm-container.js (right):

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:23: Y.WidgetParent
On 2013/03/28 15:20:10, jeff.pihach wrote:
> Just an FYI the widget parent/child stuff hasn't been worked on in a very long
> time and is likely due for a complete refactoring 

Well, should they refactor it, we'll have to deal, but we don't reach into its
internals too much in this. Thanks for the heads up.

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:37: total = this._items.length,
On 2013/03/28 15:20:10, jeff.pihach wrote:
> widget parent has a size() method
> http://yuilibrary.com/yui/docs/api/classes/WidgetParent.html#method_size

So it does, thanks!

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:49: var cut_items =
this._items.slice(this.get('cutoff'), this.get('total'));
On 2013/03/28 15:20:10, jeff.pihach wrote:
> cutItems ;)

As much as I want to comply, I'm going to keep screwing this up. It's just
unnatural. :-P

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:79: var msg = Y.Lang.sub('See {extra} more',
{extra: this.get('extra')});
On 2013/03/28 15:20:10, jeff.pihach wrote:
> In charm-container.handlebars you have this identical string. Possible bug?

No, its because we need to swap that string back in as it toggles. I don't know
of an easier way to do that.

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:94: this._events.push(expander.on('click',
this._toggleExpand, this));
On 2013/03/28 15:20:10, jeff.pihach wrote:
> If you use a property and not a attribute here then you will share events
across
> all instances of this module.

I'm not sure I follow--don't I only want this event to work on a given instance?
e.g. If I click this, I don't want a different container to expand.

I'm sure I just don't understand your point.

https://codereview.appspot.com/8080043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:137: all_visible: {
On 2013/03/28 15:20:10, jeff.pihach wrote:
> allVisible :)
Fixed.

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

https://codereview.appspot.com/8080043/diff/1/lib/views/browser/charm-small.l...
lib/views/browser/charm-small.less:6: div#main:hover button.hidden {
On 2013/03/28 15:20:10, jeff.pihach wrote:
> I don't think having a nested ID a very good idea - then you can only ever
have
> one instance of yui3-charmsmall.

I'm not actually sure where that came from; I was just refactoring this into
nested less. Was it safe the other way? If not, how else should we put this? A
CSS master I am not.

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js
File test/test_charm_container.js (right):

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js#ne...
test/test_charm_container.js:23: container.remove(true);
On 2013/03/28 15:20:10, jeff.pihach wrote:
> remove(true) doesn't call destroy(true) (unfortunately) so a more thorough way
> to destroy this would be `container.remove().destroy(true)` - I just found
this
> out yesterday. :-D

Thanks, that's fixed.

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js#ne...
test/test_charm_container.js:25: charm_container.destroy();
On 2013/03/28 15:20:10, jeff.pihach wrote:
> destroy(true) to remove all event listeners from children as well.

Done.

https://codereview.appspot.com/8080043/diff/1/test/test_charm_container.js#ne...
test/test_charm_container.js:30: charm_container = new
Y.juju.widgets.browser.CharmContainer({
On 2013/03/28 15:20:10, jeff.pihach wrote:
> the instances of CharmContainer() are never being destroyed.

They are; you actually commented on where the destroy is being called in
afterEach that it should be destroy(true) to get children.

[j.c.sackett, 2013-03-28 16:57:42]

Please take a look.

[jeff.pihach, 2013-03-28 17:02:51]

LGTM - Thanks for making those changes :)

[rharding, 2013-03-28 17:24:16]

lgtm but I'd like to make sure we square away _events as a ATTR before landing
please.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container.js
File app/widgets/charm-container.js (right):

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:74: var visible = this.get('allVisible'),
My preference here would be to actually check for hidden items via catching the
$CLASSNAME-hidden class. I'd rather check against what 'is' vs what we 'think'.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:78: var msg = Y.Lang.sub('See {extra} more',
{extra: this.get('extra')});
please move the string up to TEMPLATE so that it's pulled out and easy to
find/change if required. extra_msg or something like that.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:82: expander.set('text', 'See less');
Same here. In the wireframes it's labeled as "See less results and I think it
makes sense to have it "See fewer results"

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:93: this.get('_events').push(expander.on('click',
this._toggleExpand, this));
why are _events an ATTR?

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:102: Y.Array.each(this._events, function(e) {
here you're using this._events vs the ATTR _events.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:113: this.get('_events').push(
more _events ATTR fun

https://codereview.appspot.com/8080043/diff/7001/test/test_charm_container.js
File test/test_charm_container.js (right):

https://codereview.appspot.com/8080043/diff/7001/test/test_charm_container.js...
test/test_charm_container.js:61: assert.equal(3, shown_charms.size());
this test is just asserting that slice works.

https://codereview.appspot.com/8080043/diff/7001/test/test_charm_container.js...
test/test_charm_container.js:62: assert.equal(1, hidden_charms.size());
same as above

[j.c.sackett, 2013-03-28 17:40:16]

Replied to your comments; changes coming.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container.js
File app/widgets/charm-container.js (right):

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:74: var visible = this.get('allVisible'),
On 2013/03/28 17:24:17, rharding wrote:
> My preference here would be to actually check for hidden items via catching
the
> $CLASSNAME-hidden class. I'd rather check against what 'is' vs what we
'think'.

Sure.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:78: var msg = Y.Lang.sub('See {extra} more',
{extra: this.get('extra')});
On 2013/03/28 17:24:17, rharding wrote:
> please move the string up to TEMPLATE so that it's pulled out and easy to
> find/change if required. extra_msg or something like that.

Works for me.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:93: this.get('_events').push(expander.on('click',
this._toggleExpand, this));
On 2013/03/28 17:24:17, rharding wrote:
> why are _events an ATTR?

See Jeff's comment on _events in the previous patch set. In short, if it's a
property instead of an _attr, the events can be shared across multiples
instances of a widget. Which can be bad/unpredictable.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:102: Y.Array.each(this._events, function(e) {
On 2013/03/28 17:24:17, rharding wrote:
> here you're using this._events vs the ATTR _events.
Thanks for catching that.

https://codereview.appspot.com/8080043/diff/7001/app/widgets/charm-container....
app/widgets/charm-container.js:113: this.get('_events').push(
On 2013/03/28 17:24:17, rharding wrote:
> more _events ATTR fun

This is actually consistent with Jeff's recommendation.

https://codereview.appspot.com/8080043/diff/7001/test/test_charm_container.js
File test/test_charm_container.js (right):

https://codereview.appspot.com/8080043/diff/7001/test/test_charm_container.js...
test/test_charm_container.js:62: assert.equal(1, hidden_charms.size());
On 2013/03/28 17:24:17, rharding wrote:
> same as above

Good point in both cases; it's a leftover from a test refactor.

[j.c.sackett, 2013-03-28 20:17:18]

Please take a look.

[rharding, 2013-03-28 20:23:29]

LGTM thanks for working through it today.

[j.c.sackett, 2013-03-28 20:34:15]

*** Submitted:

Adds charm container widget.

-Adds a new widget, CharmContainer. CharmContainer accepts CharmSmall widgets
as children. It has a set cutoff which is the maximum number of items it will
intially show, and an expander action that shows/hides the remaining items. It
can be named, and is meant to be used to loosely categorize a group of charms,
e.g. "Popular", "New".

R=jeff.pihach, rharding
CC=
https://codereview.appspot.com/8080043