Create a view for the browser charm details.

Create a view for the browser charm details.

- Drive by fixes for some missing deps
- Drive by for the api url in the prod config
- Refactory out the logic of the charm details from Fullscreen to it's own
view so it can be reused hopefully in the Sidebar View as well.
- Add the icon to the BrowserCharm model since it's new.
- Add the `file` Charmworld API endpoint used for loading the readme contents
if available.
- Fix broken HTML heh
- Add events with place holder functions for clicking 'add' and 'open
changelog'.
- Add basic tests for the BrowserCharmView
- If you want to QA, go to /bws/sidebar and select one of the charm links
displayed. It will load details for that charm. Note: there's an upstream
Charmworld API issue if the charm *does* have a README file. It'll throw an
error currently and a fix to Charmworld is submitted but not in staging yet.

https://code.launchpad.net/~rharding/juju-gui/clean_charmview/+merge/156054

(do not edit description out of merge proposal)

[rharding, 2013-03-28 23:34:41]

Please take a look.

[jeff.pihach, 2013-03-29 03:26:31]

Thanks for this, it is looking good! I am starting to get a little concerned
about all of the ID's that I see popping up as this stuff grows. This could
cause odd, difficult to debug issues in the future. Could you look into either
namespacing the required id's or converting them to classes?

Thanks, talk to you tomorrow!

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

https://codereview.appspot.com/8103046/diff/1/app/store/charm.js#newcode210
app/store/charm.js:210: * for.
missing a couple params here.

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/templates/b...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/templates/b...
app/subapps/browser/templates/browser_charm.handlebars:54: <div
id="readme">Readme</div>
these id's are pretty general to avoid ID clashes in the future we should throw
something like bws in front of them.

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:139: this.tabview.render();
We should probably be destroying this in a destructor.

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:145: this._loadFile(tplNode.one('#readme'),
Are there going to be multiple instances of this template? If so there will be
ID clashing. Could we use a class?

[rharding, 2013-03-29 11:15:35]

Thanks for the feedback. Comments below. Updated code coming soon.

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

https://codereview.appspot.com/8103046/diff/1/app/store/charm.js#newcode210
app/store/charm.js:210: * for.
On 2013/03/29 03:26:31, jeff.pihach wrote:
> missing a couple params here.

Doh, updating.

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/templates/b...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/templates/b...
app/subapps/browser/templates/browser_charm.handlebars:54: <div
id="readme">Readme</div>
On 2013/03/29 03:26:31, jeff.pihach wrote:
> these id's are pretty general to avoid ID clashes in the future we should
throw
> something like bws in front of them.

Yep, will do. The tabview seems to want ids. I'll bws_ them.

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:139: this.tabview.render();
On 2013/03/29 03:26:31, jeff.pihach wrote:
> We should probably be destroying this in a destructor.

Yep.

https://codereview.appspot.com/8103046/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:145: this._loadFile(tplNode.one('#readme'),
On 2013/03/29 03:26:31, jeff.pihach wrote:
> Are there going to be multiple instances of this template? If so there will be
> ID clashing. Could we use a class?

No, the case that might come close is when this is used for the sidebar and the
fullscreen. When we get that I'll have to make sure it's getting destroyed.
Since the route isn't preserved I don't *think* we should get dupes, but I'll
want to verify and test when it's added to the sidebar.

[rharding, 2013-03-29 12:35:45]

Please take a look.

[j.c.sackett, 2013-03-29 13:28:47]

Looks good, Rick; a few questions below.

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:46: },
This may be premature, but there's a CHARM_ADD_EVT in charm-small for its add
button; perhaps this should fire that same event, and something higher up handle
it in either event?

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:103: },
This is bikeshedding, but would it be better to say "Charm had no readme." ?

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:127: },
Are events in a view automatically handled when destroy is called on it? Or do
you need to detach them here?

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:161:
this._noReadme(tplNode.one('#bws_readme'));
Reading this, it almost seems like locateReadme should just take success and
failure handlers, since that's basically what's happening here.

I suppose this helps with testing, since you can test _locateReadme separately?

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/fu...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/fu...
app/subapps/browser/views/fullscreen.js:71: if (!Y.Lang.isValue(container)) {
I thought we weren't using Y.Lang.isValue? Or do you need it here b/c `if
(!container)` won't work?

[j.c.sackett, 2013-03-29 13:39:49]

LGTM. Rick, thanks for answering my questions in our hangout.

[jeff.pihach, 2013-03-29 15:22:16]

LGTM - Thanks for those fixes!

[rharding, 2013-03-29 15:41:03]

*** Submitted:

Create a view for the browser charm details.

- Drive by fixes for some missing deps
- Drive by for the api url in the prod config
- Refactory out the logic of the charm details from Fullscreen to it's own
view so it can be reused hopefully in the Sidebar View as well.
- Add the icon to the BrowserCharm model since it's new.
- Add the `file` Charmworld API endpoint used for loading the readme contents
if available.
- Fix broken HTML heh
- Add events with place holder functions for clicking 'add' and 'open
changelog'.
- Add basic tests for the BrowserCharmView
- If you want to QA, go to /bws/sidebar and select one of the charm links
displayed. It will load details for that charm. Note: there's an upstream
Charmworld API issue if the charm *does* have a README file. It'll throw an
error currently and a fix to Charmworld is submitted but not in staging yet.

R=jeff.pihach, j.c.sackett
CC=
https://codereview.appspot.com/8103046

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/8103046/diff/8001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:46: },
On 2013/03/29 13:28:47, j.c.sackett wrote:
> This may be premature, but there's a CHARM_ADD_EVT in charm-small for its add
> button; perhaps this should fire that same event, and something higher up
handle
> it in either event?

Yea, it'll have to be coordinated at some point. I think when we add something
in the view to catch it we'll sync up. Maybe move the definition somewhere
shared.