Code review comment for lp:~rharding/juju-gui/update-urls

Revision history for this message
j.c.sackett (jcsackett) wrote :

On 2013/04/15 22:15:24, rharding wrote:
> 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.

Ok. Can you XXX or something to that effect?

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

Ok. This still feels weird to me, but I understand it. Thanks.

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

I think it's fine as is, I was just double checking my understanding.

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

Thanks for fixing!

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

You can just XXX it for now and get it later, if you like. I just want
to make sure it gets handled.

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

Ok, thanks.

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

« Back to merge proposal