Merge lp://staging/~gary/juju-gui/relatedcharms into lp://staging/juju-gui/experimental

Reviewers: mp+133004_code.launchpad.net,

Message:
Please take a look.

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.

Tests for the view code are not factored as I had intended: I wanted
stubs to
test the composite functions in isolation. Kapil nixed this in favor of
test-all-the-way-through test functions as I have them here.

Thanks.

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

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/store/charm.js
   A app/templates/charm-description-related.handlebars
   M app/templates/charm-description.handlebars
   M app/views/charm-panel.js
   M lib/views/stylesheet.less
   M test/test_charm_panel.js
   M test/test_charm_store.js
   M undocumented

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-related.handlebars
File app/templates/charm-description-related.handlebars (right):

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description-related.handlebars#newcode9
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 inserte...

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

https://codereview.appspot.com/6814089/

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.

https://codereview.appspot.com/6814089/

*** 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

https://codereview.appspot.com/6814089/