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

Revision history for this message
Leonard Richardson (leonardr) wrote :

After a lot of questions I've just got a tech-debt bug that needs filing, a question, and some wording suggestions. I also have some questions I'd like the UI reviewer to answer.

The tech-debt bug: you copy a big chunk of code into activate_collapsible_div. We talked about this on IRC and you explained that you'd tried refactoring, but changing the code to work on <div> instead of <fieldset> made it a huge pain. We agreed that you'd file a tech-debt bug and put an XXX in the code.

The question: I don't know if it makes sense to use waitForPageLoad when the page has already loaded. Is there a better way to wait for some Javascript to run? If so, please change this.

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

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.
2. "Configuration links" is a little vague and passive. Maybe "Configure this project" would be better wording?

review: Approve (code)

« Back to merge proposal