Handles edge conditions for charm container

Handles edge conditions for charm container

The charm container previously could end up in an awkward situation if the
cutoff was higher than the number of charms passed in. This addresses that
situation.

-Cutoff must be a nonnegative number
-The expander control only renders if there are extra charm tokens; in other
words, only renders if more charms are passed in than the cutoff.
-The expand event is only created and bound if the same condition is met.
-As a side effect, the container is now made safe if no charms are passed in
at all.

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

(do not edit description out of merge proposal)

[j.c.sackett, 2013-04-03 12:58:48]

Please take a look.

[rharding, 2013-04-03 13:58:46]

LGTM with trivial below.

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

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:42: this.set('has_extra', false);
I'd change this to a getter on the attr has_extra. Then you don't need to keep
it in sync. 

http://yuilibrary.com/yui/docs/attribute/#subattrs-gsv

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:95: if (this.get('extra') > 0) {
if this.get('has_extra')

https://codereview.appspot.com/8317043/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/8317043/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://0.0.0.0:8084/test/index.html
please revert this since it breaks some dev's instances.

[j.c.sackett, 2013-04-03 14:32:22]

Thanks. Comments addressed, changes coming.

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

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:42: this.set('has_extra', false);
Done.

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:95: if (this.get('extra') > 0) {
Done.

https://codereview.appspot.com/8317043/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/8317043/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://0.0.0.0:8084/test/index.html
Five times I remember to keep this out of the commit; then when I'm submitting
*of course* I forget. :-P

Reverted.

[jeff.pihach, 2013-04-03 14:38:27]

Thanks for these changes - for some reason this test suite causes our tests to
fail more frequently - maybe these changes will fix that.

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

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:177: has_extra: {
What advantage do we get from having this attribute and the overhead which comes
with it vs `this.get('extra').length` ?

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:195: 'base',
you should also add base-build in here as it's required for the create method.

[j.c.sackett, 2013-04-03 14:59:32]

Please take a look.

[jeff.pihach, 2013-04-03 15:04:54]

See zee comment below.

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

https://codereview.appspot.com/8317043/diff/8001/app/widgets/charm-container....
app/widgets/charm-container.js:172: has_extra: {
I'm really not a fan of having attributes who's sole purpose is to return
another attributes value. Those are getter utility methods which should be in
the prototype. This adds unnecessary attribute overhead. Thoughts?

[j.c.sackett, 2013-04-03 15:16:40]

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

https://codereview.appspot.com/8317043/diff/1/app/widgets/charm-container.js#...
app/widgets/charm-container.js:177: has_extra: {
On 2013/04/03 14:38:27, jeff.pihach wrote:
> What advantage do we get from having this attribute and the overhead which
comes
> with it vs `this.get('extra').length` ?

Being able to pass it into the template via this.getAttrs, rather than having to
compile a data object.

[j.c.sackett, 2013-04-03 15:18:09]

Please take a look.

[jeff.pihach, 2013-04-03 15:19:43]

LGTM - Ahh well in that case that should be a handlebars helper. But I'm going
to approve this to keep things moving forward just keep that in mind in the
future :-)

[j.c.sackett, 2013-04-03 15:48:04]

On 2013/04/03 15:19:43, jeff.pihach wrote:
> LGTM - Ahh well in that case that should be a handlebars helper. But I'm going
> to approve this to keep things moving forward just keep that in mind in the
> future :-)

Ok, thanks.

[j.c.sackett, 2013-04-03 17:09:10]

*** Submitted:

Handles edge conditions for charm container

The charm container previously could end up in an awkward situation if the
cutoff was higher than the number of charms passed in. This addresses that
situation.

-Cutoff must be a nonnegative number
-The expander control only renders if there are extra charm tokens; in other
words, only renders if more charms are passed in than the cutoff.
-The expand event is only created and bound if the same condition is met.
-As a side effect, the container is now made safe if no charms are passed in
at all.

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