Code review comment for lp:~jcsackett/launchpad/hidden-project-configuration-636000

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

I'm only addressing part of the review in this comment--I'm investigating some things related to the other parts. In particular I may be able to remove the tech debt after all. Funny how things change when you look at code again.

> Wording suggestions:
>
> 1. s/"safe use this"/"safely use this"
> 2. In your explanatory text, you need to distinguish between the configuration links and the collapsible element that *contains* the configuration links.
>
> 2a. # When the Show link is clicked when it's open, it closes it.
> =>
> # When the collapsable element is open, clicking the Show link will close it.
>
> 2b. "Additionally, on a not fully configured project, it starts by showing
> the links, and can be closed."
>
> =>
>
> "On a not fully configured project, the collapsable element starts off
> open and displaying the configuration links, but it can be closed."

I've updated the comments.

> Questions that I defer to the UI reviewer:
>
> 1. Do you need that <br /> on line 270? If so, maybe it should also be conditional on view/configuration_links? A <br /> with nothing after it doesn't look good.

I've killed the br. It was there b/c I thought the spacing looked a little off when I was cleaning this up for review, but on second thought I think it may be better without. So I suppose the new question for the UI reviewer is "do you have suggestions for spacing between the show/hide toggle and the actual links?"

> 2. "Configuration links" is a little vague and passive. Maybe "Configure this project" would be better wording?

I borrowed the passivity from other collapse toggles on the site. Most places we use it just show "Extra options" as the link (e.g. merge proposals). I'm not in love with the language at all. Suggestion from the UI review heartily encouraged.

« Back to merge proposal