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

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Looks great Brad!

The only thing that I think could be improved is the spacing between the spans. Currently the displayname of each mugshot is wedged in between two mugshots. If there were 32 mugshots on the page, after scrolling it would be more difficult to recognise which displayname belonged to which mugshot.

I'd suggest adding:
 padding: 1em 1em 1em 0;
to the .gridflow li span. It seems to provide enough space inside each span so that the displayname is clearly associated with the mugshot above it. See what you think.

Oh, and I realise it wasn't changed as part of this branch, but is "Who's in this team?" a good h1/breadcrumb? Given that the link that I clicked on was "Show member photos" perhaps "Member photos" would be better?

... and one css-style point: you've indented the css with 8 spaces rather than 4.

Thanks!

review: Approve (ui*)

« Back to merge proposal