Add the required charm filter dropdown.

Add the required charm filter dropdown.

The original design for the charm search results panel had a dropdown
that was to be used to filter the results.  After some consideration
the three sets were chosen to be 'all', 'deployed', and 'subordinate'.

This branch implements the dropdown and filter mechanism.  The widget
was based on the graph/list picker on the bottom of the page.  It was
refactored into a generic LESS function and supports both uses.

The design for the existing picker had a 1px wide design element to
provide the background.  It was not tall enough for a picker with
three items.  As a work-around an 'outset' mitred HTML border is
used.  It will be easy to replace with the proper asset when it is
produced.

If the picker is open and a click outside closes the charm panel then
the picker remains open.  That is a wart that needs to be fixed.

https://code.launchpad.net/~bac/juju-gui/cs-filter/+merge/137050

(do not edit description out of merge proposal)

[bac, 2012-11-29 21:08:41]

Please take a look.

[benji, 2012-11-30 20:37:47]

This branch looks good.  Tests are especially appreciated.  I had many small
comments.  Given the number of comments I would like to do a quick re-review
after they are addressed.

https://codereview.appspot.com/6842119/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/6842119/diff/1/app/store/charm.js#newcode74
app/store/charm.js:74: is_subordinate: result.subordinate});
I guess this name uses underscores because the rest of the file does.  I wonder
if we need an underscore-geddon.

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:6: <li class="picker-item all"
data-filter="all">All charms ({{all_charms_count}})</li>
The use of "data" attributes is nice.

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:9: </ul>
There are some long lines here.

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

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode85
app/views/charm-panel.js:85: var deployed_charms,
We have recently switched to one-var-per-variable.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode93
app/views/charm-panel.js:93: is_sub_filter = function(charm) {
I think this file uses enough camel case that these new variables should be
camel case too.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:134: series_group.charms = sub_charms;
If you wanted, these two lines could be combined and the "sub_charms" variable
removed.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:257: //this.render();
Commented code accidentally left in.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:276: if (true) {             // Avoid lint warning.
Ignorable punctiliousness: that's a whole lot of spaces before that comment.

https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:692: /* XXX bac: The positioning is off but I have
been unable
Long lines.

https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:760: /*#zoom-btn-up {
Commented-out code.

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:324: Y.one('#main').append(Y.Node.create(
We have transitioned to using the chained node methods to create DOM nodes
instead of strings with HTML in them.

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:328: container = Y.Node.create('<div
id="test-container"></div>');
This node probably doesn't need the ID.  Also, we have started hiding the test
container if the code under test still works with it hidden, that way the test
runner is a bit easier on the eyes.  If not, we use "visibility: hidden" so as
to keep it as clean as possible.

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:335: charm_store = new juju.CharmStore(
We recently changed the style guide for object literals.  The style guide has
details.

[frankban, 2012-12-03 14:09:13]

This branch looks good Brad, thanks.
Approved with changes... well, with answers at least.

I am a little bit confused by some weird behaviors I've found while running the
server using this branch and the rapi-rollup sample.json.
Maybe it's just my cache or some local problem, or maybe you will be able to
reproduce the following:

1) The charm panel's height seems wrong: I see the panel partially over the
bottom bar.

2) By default, clicking on ``Charms``, I only see one deployed service. I
expected to see five of them (the five ones I see in the env view).

3) Searching for ``mysql`` the number of deployed services in the resulting
panel is ten. I expected to only see the one that's actually deployed on the
environment.

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:5: <ul>
Is there a reason here (and, I guess, in the graph/list picker at the bottom of
the page) for not using the more conventional HTML <select> widget?

[bac, 2012-12-03 15:05:22]

Thanks for the review Benji.  I think I've addressed all of your concerns. 
After I respond to Francesco's concerns I'll push a new proposal.

https://codereview.appspot.com/6842119/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/6842119/diff/1/app/store/charm.js#newcode74
app/store/charm.js:74: is_subordinate: result.subordinate});
is_subordinate is a pre-existing property on a charm.  Yeah, we may need an
effort to bring them all in-line but I'd rather not change existing stuff now.

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:6: <li class="picker-item all"
data-filter="all">All charms ({{all_charms_count}})</li>
On 2012/11/30 20:37:47, benji wrote:
> The use of "data" attributes is nice.

I learned it from you!  :)

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:9: </ul>
On 2012/11/30 20:37:47, benji wrote:
> There are some long lines here.

Fixed except as noted below.

https://codereview.appspot.com/6842119/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:33: href="{{id}}">{{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
This line is 79, it needs to remain due to the comment above.  I  could have the
view export a variable to get around the logic here but that seems extreme to
just avoid a longish line.

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

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode85
app/views/charm-panel.js:85: var deployed_charms,
On 2012/11/30 20:37:47, benji wrote:
> We have recently switched to one-var-per-variable.

Done.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcode93
app/views/charm-panel.js:93: is_sub_filter = function(charm) {
On 2012/11/30 20:37:47, benji wrote:
> I think this file uses enough camel case that these new variables should be
> camel case too.

Done.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:134: series_group.charms = sub_charms;
On 2012/11/30 20:37:47, benji wrote:
> If you wanted, these two lines could be combined and the "sub_charms" variable
> removed.

Done.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:257: //this.render();
On 2012/11/30 20:37:47, benji wrote:
> Commented code accidentally left in.

Entire handler is removed.

https://codereview.appspot.com/6842119/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:276: if (true) {             // Avoid lint warning.
On 2012/11/30 20:37:47, benji wrote:
> Ignorable punctiliousness: that's a whole lot of spaces before that comment.

Done.

https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:692: /* XXX bac: The positioning is off but I have
been unable
On 2012/11/30 20:37:47, benji wrote:
> Long lines.

Done.

https://codereview.appspot.com/6842119/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:760: /*#zoom-btn-up {
On 2012/11/30 20:37:47, benji wrote:
> Commented-out code.

This code is not new, just refactored, so I left in the commented code.  I've
removed it now.

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:324: Y.one('#main').append(Y.Node.create(
On 2012/11/30 20:37:47, benji wrote:
> We have transitioned to using the chained node methods to create DOM nodes
> instead of strings with HTML in them.

Done.

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:328: container = Y.Node.create('<div
id="test-container"></div>');
On 2012/11/30 20:37:47, benji wrote:
> This node probably doesn't need the ID.  Also, we have started hiding the test
> container if the code under test still works with it hidden, that way the test
> runner is a bit easier on the eyes.  If not, we use "visibility: hidden" so as
> to keep it as clean as possible.

Done.

https://codereview.appspot.com/6842119/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:335: charm_store = new juju.CharmStore(
OK, thanks for your discussion on IRC.  I have reformatted according to the new
proposed rules.  I'm mildly negative on those changes, though, as I'd prefer
visibility to block closure over saving of vertical space.

[bac, 2012-12-03 20:10:05]

On 2012/12/03 14:09:13, frankban wrote:
> This branch looks good Brad, thanks.
> Approved with changes... well, with answers at least.
> 
> I am a little bit confused by some weird behaviors I've found while running
the
> server using this branch and the rapi-rollup sample.json.
> Maybe it's just my cache or some local problem, or maybe you will be able to
> reproduce the following:
> 
> 1) The charm panel's height seems wrong: I see the panel partially over the
> bottom bar.

This is a bug caused by the addition of the filter widget in one view of the
charm panel.  It has been fixed.  Thanks for catching it.

> 
> 2) By default, clicking on ``Charms``, I only see one deployed service. I
> expected to see five of them (the five ones I see in the env view).

This is a problem with the data served up by improv not having the current
versions.  I'm talking to Kapil about a solution.

> 
> 3) Searching for ``mysql`` the number of deployed services in the resulting
> panel is ten. I expected to only see the one that's actually deployed on the
> environment.

Your expectation matches what Jovan has requested though I interpreted it
differently.  Changed to only match if the charm URL is the same.

[bac, 2012-12-03 20:43:29]

Please take a look.

[benji, 2012-12-03 20:58:15]

Looks good.  I had a couple of small questions, mostly for my own edification.

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#ne...
app/views/charm-panel.js:82: * @return {Array} A filtered, grouped set of
filtered entries.
So, is the return value filtered?  ;P

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#ne...
app/views/charm-panel.js:93: var isSubFilter = function(charm) {
Why not just "function isSubFilter"?  Oh, wait, I think there is a bug in some
browsers that makes that not work right.  Is that it?

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#ne...
app/views/charm-panel.js:980: // "charms" panel.
I don't feel strongly one way or the other (or both, even), but I thought I
would point out that some of the code in this branch uses /* */ style comments
and some uses // comments.

https://codereview.appspot.com/6842119/diff/12001/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6842119/diff/12001/test/test_charm_panel.js#ne...
test/test_charm_panel.js:326: .setAttribute('id', 'charm-search-test').append(
Since this code is chaining method calls, shouldn't the ".append" be the first
code on a new line?  Like so: http://paste.ubuntu.com/1408800/

[bac, 2012-12-03 22:24:00]

Thanks for the re-review Benji.

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#ne...
app/views/charm-panel.js:82: * @return {Array} A filtered, grouped set of
filtered entries.
On 2012/12/03 20:58:15, benji wrote:
> So, is the return value filtered?  ;P

YES, I wanted to make sure it was well known!

fixed

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#ne...
app/views/charm-panel.js:93: var isSubFilter = function(charm) {
On 2012/12/03 20:58:15, benji wrote:
> Why not just "function isSubFilter"?  Oh, wait, I think there is a bug in some
> browsers that makes that not work right.  Is that it?

no, it was just plain silliness.

fixed.  we'll see now if i break those browsers.

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#ne...
app/views/charm-panel.js:980: // "charms" panel.
On 2012/12/03 20:58:15, benji wrote:
> I don't feel strongly one way or the other (or both, even), but I thought I
> would point out that some of the code in this branch uses /* */ style comments
> and some uses // comments.

I'll fix it so that YUIDOC are /** */ and others are //.

https://codereview.appspot.com/6842119/diff/12001/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6842119/diff/12001/test/test_charm_panel.js#ne...
test/test_charm_panel.js:326: .setAttribute('id', 'charm-search-test').append(
On 2012/12/03 20:58:15, benji wrote:
> Since this code is chaining method calls, shouldn't the ".append" be the first
> code on a new line?  Like so: http://paste.ubuntu.com/1408800/

Done.

[bac, 2012-12-03 22:28:07]

*** Submitted:

Add the required charm filter dropdown.

The original design for the charm search results panel had a dropdown
that was to be used to filter the results.  After some consideration
the three sets were chosen to be 'all', 'deployed', and 'subordinate'.

This branch implements the dropdown and filter mechanism.  The widget
was based on the graph/list picker on the bottom of the page.  It was
refactored into a generic LESS function and supports both uses.

The design for the existing picker had a 1px wide design element to
provide the background.  It was not tall enough for a picker with
three items.  As a work-around an 'outset' mitred HTML border is
used.  It will be easy to replace with the proper asset when it is
produced.

If the picker is open and a click outside closes the charm panel then
the picker remains open.  That is a wart that needs to be fixed.

R=benji, frankban
CC=
https://codereview.appspot.com/6842119