Add related charms section to charm description

Add related charms section to charm description

This adds a related charms section to the charm panel, per UX design.

It adds new functionality to the charm store find method in order to make
the search efficient.

https://code.launchpad.net/~gary/juju-gui/relatedcharms/+merge/133004

(do not edit description out of merge proposal)

[gary.poster, 2012-11-06 02:11:31]

Please take a look.

[benji, 2012-11-06 14:49:55]

Looks good.  I spotted a few small things that you'll want to correct, but after
that this looks good to go.

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description...
File app/templates/charm-description-related.handlebars (right):

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description...
app/templates/charm-description-related.handlebars:9: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
Is the "if" redundant?  I /think/ handlebars will omit the value if it is null
or undefined.

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

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode65
app/views/charm-panel.js:65: /**
Our other multi-line yuidoc comments have a row of asterisks down the left.  Do
we want to settle on one style or the other?

My vote would be for asterisks as it helps the comments stand out a bit.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode71
app/views/charm-panel.js:71: @static
I'm curious.  What does "static" mean here?  Is it that this method doesn't
access "this"?

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode80
app/views/charm-panel.js:80: return {
This isn't the object literal style we use elsewhere, but I suspect the normal
style won't work here because of JavaScript's rules for implicitly placing
semicolons.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode87
app/views/charm-panel.js:87: /**
I find the Roman dislike for helpful vertical whitespace bothersome.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:342: @method
This directive needs the method name.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:374: If there was a failure, render it to the console
and to the
This comment and the one just above have different indentation styles.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:401: @method
This needs the method name.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:431: Fires an event indicating that the charm panel
should switch to the
<nitpick level="extreme" mode="prose advice from a programmer">I can't explain
why exactly, but removing the "s" from "Fires an event" makes this much better. 
More "active" I guess.</nitpick>

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

https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:278: assert(section_container.one('h4
i').hasClass('icon-chevron-up'));
Not your fault: The above is rife with brittle selectors.  It would be great to
fix this up.

[gary.poster, 2012-11-06 18:35:46]

Thank you for the review, Benji.  I have made the changes that you requested
except for one or two that I discuss below.  Feel free to reply if you are still
concerned about something that I push back on, or explain/rationalize
unsatisfactorily.

Gary

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description...
File app/templates/charm-description-related.handlebars (right):

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description...
app/templates/charm-description-related.handlebars:9: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
On 2012/11/06 14:49:55, benji wrote:
> Is the "if" redundant?  I /think/ handlebars will omit the value if it is null
> or undefined.
Notice the "/" after {{owner}}.

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

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode65
app/views/charm-panel.js:65: /**
On 2012/11/06 14:49:55, benji wrote:
> Our other multi-line yuidoc comments have a row of asterisks down the left. 
Do
> we want to settle on one style or the other?
> 
> My vote would be for asterisks as it helps the comments stand out a bit.
OK, will do.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode71
app/views/charm-panel.js:71: @static
On 2012/11/06 14:49:55, benji wrote:
> I'm curious.  What does "static" mean here?  Is it that this method doesn't
> access "this"?
Remember I mentioned on a call that yuidoc has no concept of standalone
functions that are not class initializers?  I found three workarounds.  First,
you can simply add comments without tags.  I believe I found that yuidoc or one
of our linters was not really happy with this.  It also seemed annoying to me
that you could not specify params and return values.  Second, Dav Glass said in
an email message I found that he would use tags to specify that this was a
class.  I found this hack deeply unsatisfying. It also produces bad docs IMO,
when I tried it.  Third, you can say that this is a method.  Of what?  Good
question.  Maybe the window.  But it is a static method, yes: it does not access
"this".

I have a card about this for the weekly retrospective.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode80
app/views/charm-panel.js:80: return {
On 2012/11/06 14:49:55, benji wrote:
> This isn't the object literal style we use elsewhere, but I suspect the normal
> style won't work here because of JavaScript's rules for implicitly placing
> semicolons.
Correct.  I could wrap it all in parentheses, but that seemed much of a muchness
to me.  What I wrote passes the linter.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode87
app/views/charm-panel.js:87: /**
On 2012/11/06 14:49:55, benji wrote:
> I find the Roman dislike for helpful vertical whitespace bothersome.
Could you make a topic about vertical whitespace for a retrospective discussion?
 I wouldn't mind vertical spaces immediately above the yuidoc comments,
separating them from the functions above them.  OTOH, I find vertical whitespace
inserted into function bodies to generally be distractingly arbitrary if you are
not the person who made the whitespace.  Unfortunately, vertical whitespace are
still a stylistic change about which we can all disagree, despite our
beautifiers and linters.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:342: @method
On 2012/11/06 14:49:55, benji wrote:
> This directive needs the method name.
Thanks, good catch.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:374: If there was a failure, render it to the console
and to the
On 2012/11/06 14:49:55, benji wrote:
> This comment and the one just above have different indentation styles.
Yeah, it's in part because the first comment is from outside the hash, and the
second one is within the hash; and possibly also in part because the beautifier
or I are insane, or because we are insane when combined together.  In any case,
I've tried to unify by putting the first comment withing the hash.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:401: @method
On 2012/11/06 14:49:55, benji wrote:
> This needs the method name.
Thanks, fixed

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcod...
app/views/charm-panel.js:431: Fires an event indicating that the charm panel
should switch to the
On 2012/11/06 14:49:55, benji wrote:
> <nitpick level="extreme" mode="prose advice from a programmer">I can't explain
> why exactly, but removing the "s" from "Fires an event" makes this much
better. 
> More "active" I guess.</nitpick>
From a consistency perspective, I do vary between an imperative style (e.g.,
"Fire") and a descriptive style (e.g., "Fires").  I tried to settle on
imperative here and elsewhere (e.g. "returns" -> "return").

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

https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:278: assert(section_container.one('h4
i').hasClass('icon-chevron-up'));
On 2012/11/06 14:49:55, benji wrote:
> Not your fault: The above is rife with brittle selectors.  It would be great
to
> fix this up.
Probably my fault one way or the other. :-P That said, I'm not clear on what to
do that would truly be better.  I could put classes on everything, but that's
fragile in a different dimension IMO.  Since you don't insist, I'm leaving it.

[benji, 2012-11-06 18:47:36]

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description...
File app/templates/charm-description-related.handlebars (right):

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description...
app/templates/charm-description-related.handlebars:9: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
On 2012/11/06 18:35:46, gary.poster wrote:
> On 2012/11/06 14:49:55, benji wrote:
> > Is the "if" redundant?  I /think/ handlebars will omit the value if it is
null
> > or undefined.
> Notice the "/" after {{owner}}.

Ah!  Gotcha.

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

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode71
app/views/charm-panel.js:71: @static
On 2012/11/06 18:35:46, gary.poster wrote:
> On 2012/11/06 14:49:55, benji wrote:
> > I'm curious.  What does "static" mean here?  Is it that this method doesn't
> > access "this"?
> Remember I mentioned on a call that yuidoc has no concept of standalone
> functions that are not class initializers?  I found three workarounds.  First,
> you can simply add comments without tags.  I believe I found that yuidoc or
one
> of our linters was not really happy with this.  It also seemed annoying to me
> that you could not specify params and return values.  Second, Dav Glass said
in
> an email message I found that he would use tags to specify that this was a
> class.  I found this hack deeply unsatisfying. It also produces bad docs IMO,
> when I tried it.  Third, you can say that this is a method.  Of what?  Good
> question.  Maybe the window.  But it is a static method, yes: it does not
access
> "this".
> 
> I have a card about this for the weekly retrospective.

Interesting.  Thanks for the info.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode80
app/views/charm-panel.js:80: return {
On 2012/11/06 18:35:46, gary.poster wrote:
> On 2012/11/06 14:49:55, benji wrote:
> > This isn't the object literal style we use elsewhere, but I suspect the
normal
> > style won't work here because of JavaScript's rules for implicitly placing
> > semicolons.
> Correct.  I could wrap it all in parentheses, but that seemed much of a
muchness
> to me.

Agreed.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode87
app/views/charm-panel.js:87: /**
On 2012/11/06 18:35:46, gary.poster wrote:
> On 2012/11/06 14:49:55, benji wrote:
> > I find the Roman dislike for helpful vertical whitespace bothersome.
> Could you make a topic about vertical whitespace for a retrospective
discussion?

Done.

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

https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js#newcod...
test/test_charm_panel.js:278: assert(section_container.one('h4
i').hasClass('icon-chevron-up'));
On 2012/11/06 18:35:46, gary.poster wrote:
> On 2012/11/06 14:49:55, benji wrote:
> > Not your fault: The above is rife with brittle selectors.  It would be great
> to
> > fix this up.
> Probably my fault one way or the other. :-P That said, I'm not clear on what
to
> do that would truly be better.  I could put classes on everything, but that's
> fragile in a different dimension IMO.  Since you don't insist, I'm leaving it.

Leaving it is fine with me.  Maybe I will remember to bring it up during our
one-on-one.

[frankban, 2012-11-07 11:56:43]

Looks good. Looking at this branch I learned a lot about how the charm panel
works. Thank you.

[gary.poster, 2012-11-07 13:17:33]

On 2012/11/07 11:56:43, francesco.banconi wrote:
> Looks good. Looking at this branch I learned a lot about how the charm panel
> works. Thank you.

Thank you for the review, Francesco.

[gary.poster, 2012-11-07 13:18:45]

*** Submitted:

Add related charms section to charm description

This adds a related charms section to the charm panel, per UX design.

It adds new functionality to the charm store find method in order to make
the search efficient.

R=benji, francesco.banconi
CC=
https://codereview.appspot.com/6814089