Code review comment for lp:~bac/launchpad/bug-652149

Revision history for this message
Henning Eggers (henninge) wrote :

Hi Brad,
this looks very good, thank you. The icon misalignment I noticed must be related to other stuff. I see it all over Launchpad now.

I was about to make suggestions about avoiding adding the side portlets and thus losing horizontal space. Horizontal space is a little issue because the project group page has an extra column on the branch listing. The information could be placed in the top portlet but I don't know how long the list of teams might get. Having a long list of team exceptions before reaching the actual branches degrades the usefulness of the page. Maybe the list could have been placed under the branch listing.

But I realized that a project's code page already has that portlet about visibility in the same spot, so I guess this is consistent. IIRC 3.0 UI design removed side portlets from all pages expect project/application home pages?

So the only think I'd like you to think about is if the extra portlet for changing the visibilty. Could that not be integrated into the other portlet by adding an edit icon at the right place?

There is one little nitpick: forbidden-no-teams.png shows some whitespace at the bottom of the upper portlet. This appears on other similar portlets, too, and is owed to the text being in a <p> tag which has margin-bottom set to 0.3em or so. Could that be avoided somehow?

But there are no real stoppers here, seeing that the use of side portlets seems to be appropriate here. Thank you!

Henning

review: Approve (ui*)

« Back to merge proposal