Comments below. Will have an updated branch in a bit.


https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (left):

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/browser.js#oldcode191
app/subapps/browser/browser.js:191: ]
On 2013/04/15 21:06:34, j.c.sackett wrote:
> Don't we need a route like "/bws/sidebar/id*/* to match things like
the readme
> and interface routes above?

Short answer: yes
Longer: this functionality never worked and needs to be tied into the
rendering state of the charm details so it'll be implemented later.

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/browser.js#newcode139
app/subapps/browser/browser.js:139: // The fullscreen view requires that
there be no editorial content if
On 2013/04/15 21:06:34, j.c.sackett wrote:
> The sequence of if statements here seem odd; can't we just check the
subpath
> thing, and set our containerID &c based on that, as that will also be
false if
> fullscreen isn't even in the URL?

> It's possible I'm just not really understanding some of this branch,
as I'm
> having trouble figuring out the situation in which we call
renderEditorial but
> don't want to renderEditorial.

Editorial is kind of a 'default' content. It's also going to be rendered
differently if it's for a fullscreen or sidebar view. Because of that we
track it's state here.

Render editorial is called manually from both sidebar() and
fullscreen(). It determines if it should go through with it based on the
current state.

/bws/sidebar/precise/apach2-2 will call renderEditorial as it's needed
to fill the sidebar while the charm details loads next to it.

/bws/fullscreen/precise/apache2-2 will also enter this path, but we
don't need to render the editorial data so we bypass it.

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/templates/browser_charm.handlebars
File app/subapps/browser/templates/browser_charm.handlebars (left):

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/templates/browser_charm.handlebars#oldcode54
app/subapps/browser/templates/browser_charm.handlebars:54: {{/if}}
On 2013/04/15 21:06:34, j.c.sackett wrote:
> Does this (and the events change in views/charm.js) have to do with
the routes
> &c change, or just a driveby?

sorry, this was part of some debugging work where I had issues with the
double render causing events to bind twice and thus clicking this would
open and close the comments at once.

In debugging I moved the event to the h3 as a whole vs just the <a> and
missed reverting it, but it works so it didn't come out.

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

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/views/charm.js#newcode366
app/subapps/browser/views/charm.js:366: * @param {Node} container the
node to insert our rendered content into.
On 2013/04/15 21:06:34, j.c.sackett wrote:
> These params don't match; _renderCharmView has "charm" and
isFullScreen, not
> container. It's getting container as an attr.

Sorry, you're correct. This was tweaked and I didn't update the
docstring. Thanks!

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/views/editorial.js
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8679045/diff/10002/app/subapps/browser/views/editorial.js#newcode126
app/subapps/browser/views/editorial.js:126: );
On 2013/04/15 21:06:34, j.c.sackett wrote:
> Given views/charm.js has this same failure as apiFailure, and we're
using it
> here as a callback, we should probably define it in one place and have
both of
> those places use it.

Yes, my goal is to move this to a view/utils but was trying to keep from
doing too much in the branch. I'll update this.

https://codereview.appspot.com/8679045/diff/10002/app/templates/charm-token.handlebars
File app/templates/charm-token.handlebars (right):

https://codereview.appspot.com/8679045/diff/10002/app/templates/charm-token.handlebars#newcode1
app/templates/charm-token.handlebars:1: <a href="/bws/sidebar/{{id}}">
On 2013/04/15 21:06:34, j.c.sackett wrote:
> Can you update the token's less file to make sure pointer type remains
normal
> now that the entire thing is a link?

This is very temp. Actually you have to carefully click in a blank area
to trigger the link right now. I want to get UX/Huw follow up to fix it
up.

https://codereview.appspot.com/8679045/