Merge lp:~jcsackett/juju-gui/charm-container-redesign into lp:juju-gui/experimental

[j.c.sackett]

Updates charm container design

-Updates template to use new expand/collapse design (arrows, no text) and show
total charm count in title
-renderUI now assembles template data, rather than adding more ATTRS and using
a one off template helper
-hasExtra template helper is removed
-Updated tests

https://codereview.appspot.com/8529043/

[j.c.sackett (jcsackett)]

Reviewers: mp+157745_code.launchpad.net,

Message:
Please take a look.

Description:
Updates charm container design

-Updates template to use new expand/collapse design (arrows, no text)
and show
total charm count in title
-renderUI now assembles template data, rather than adding more ATTRS and
using
a one off template helper
-hasExtra template helper is removed
-Updated tests

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

Requires:
https://code.launchpad.net/~jcsackett/juju-gui/helpers-and-friends/+merge/157560

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/templates/charm-container.handlebars
   M app/widgets/charm-container.js
   M test/test_charm_container.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test/test_charm_container.js
=== modified file 'test/test_charm_container.js'
--- test/test_charm_container.js 2013-04-08 19:15:00 +0000
+++ test/test_charm_container.js 2013-04-08 19:33:53 +0000
@@ -80,7 +80,7 @@
        }]
      });
      charm_container.render(container);
- assert.equal('Popular', container.one('h3').get('text'));
+ assert.equal('Popular (4)', container.one('h3').get('text'));
      assert.isFalse(container.one('.more').hasClass('hidden'));
      assert.isTrue(container.one('.less').hasClass('hidden'));
    });
@@ -88,7 +88,6 @@
    it('toggles between all or a just few items being shown', function() {
      var hidden;
      charm_container = new Y.juju.widgets.browser.CharmContainer({
- name: 'Popular',
        children: [{
          name: 'foo'
        },{
@@ -122,7 +121,7 @@
      charm_container = new
Y.juju.widgets.browser.CharmContainer({name: 'Foo'});
      charm_container.render(container);
      var rendered = container.one('.yui3-charmcontainer');
- assert.equal('Foo', rendered.one('h3').get('text'));
+ assert.equal('Foo (0)', rendered.one('h3').get('text'));
    });

    it('handles having less charms tokens than its cutoff', function() {
@@ -142,7 +141,7 @@
      charm_container.render(container);

      var rendered = container.one('.yui3-charmcontainer');
- assert.equal('Popular', rendered.one('h3').get('text'));
+ assert.equal('Popular (4)', rendered.one('h3').get('text'));
      assert.equal(4, container.all('.yui3-charmtoken').size());
      assert.equal(0, container.all('.yui3-charmtoken-hidden').size());
      assert.equal(1, charm_container._events.length);

Index: app/templates/charm-container.handlebars
=== modified file 'app/templates/charm-container.handlebars'
--- app/templates/charm-container.handlebars 2013-04-08 19:15:00 +0000
+++ app/templates/charm-container.handlebars 2013-04-08 19:33:53 +0000
@@ -1,15 +1,15 @@
  <div>
- <h3>{{ name }}</h3>
- {{#hasExtra}}
+ <h3>{{ name }} ({{ total }})</h3>
+ {{#if hasExtra}}
    <div class="e...

[Richard Harding (rharding)]

LGTM with one trivial.

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

https://codereview.appspot.com/8529043/diff/1/app/widgets/charm-container.js#newcode130
app/widgets/charm-container.js:130: content = this.TEMPLATE(data),
this alignment is off. The guidelines for code say that a var that's
multiple lines should be on it's own. So in this case I'd break this
into multiple vars and adjust the alignment.

https://codereview.appspot.com/8529043/

[lp:~jcsackett/juju-gui/charm-container-redesign]

504. By j.c.sackett on 2013-04-08

Changes from review.

[j.c.sackett (jcsackett)]

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

https://codereview.appspot.com/8529043/diff/1/app/widgets/charm-container.js#newcode130
app/widgets/charm-container.js:130: content = this.TEMPLATE(data),
On 2013/04/08 20:46:04, rharding wrote:
> this alignment is off. The guidelines for code say that a var that's
multiple
> lines should be on it's own. So in this case I'd break this into
multiple vars
> and adjust the alignment.

Done.

https://codereview.appspot.com/8529043/

[j.c.sackett (jcsackett)]

Please take a look.

https://codereview.appspot.com/8529043/

[Jeff Pihach (hatch)]

LGTM thanks

https://codereview.appspot.com/8529043/

[lp:~jcsackett/juju-gui/charm-container-redesign]

505. By j.c.sackett on 2013-04-09

Merged helpers-and-friends into charm-container-redesign.

506. By j.c.sackett on 2013-04-09

Merged trunk.

[j.c.sackett (jcsackett)]

*** Submitted:

Updates charm container design

-Updates template to use new expand/collapse design (arrows, no text)
and show
total charm count in title
-renderUI now assembles template data, rather than adding more ATTRS and
using
a one off template helper
-hasExtra template helper is removed
-Updated tests

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

https://codereview.appspot.com/8529043/