Code review comment for lp:~michael.nelson/launchpad/distro-series-difference-browser2

On Tue, Sep 7, 2010 at 6:52 PM, Henning Eggers
<email address hidden> wrote:
> Review: Approve ui*
> Hello Michael!
> Thank you for pointing me to that ML discussion which I had missed. I see that there is a lot of thought in this page already. That's why I cannot find any big faults with it, I guess ... ;-)
>
> We discussed the following on IRC:
> - The comment should be truncated to make sure it does not exceed two lines. Looks like you already did that. ;-)

Yep, done (50 chars).

> - "no signer" should be changed to "unknown" which is clearer.

Based on the discussion you had with Paul, I've removed it when
unknown, and updated the column header.

I also updated the display of the first comment as per your discussion
with Paul, with a minor difference. I moved the "17 hours ago by Joe
Smith" to the end of the comment rather than the start, put it on a
new line, and greyed it out, you can see the differences here:

http://launchpadlibrarian.net/55200786/627295-ui-tweaks.png

> - I am not sure an extra column for the package name would really look bad but I also like the current solution ("Warty package/Hoary version"). If users get it as quickly as I did, you can leave it like that. ;)

I've switched it back and added an extra Source column (we'd only done
the combined column in the first place to save space, but that was
before we later decided on moving info into a drop-down).

>
> Not discussed:
> - The "-" sign in "foo - 1.17.1" should go away, I think. Look at a page like this https://edge.launchpad.net/ubuntu/maverick/+package/bash to see that name and version are simply separated by a space. Or provide an extra column ... ;-)

FYI: I wasn't putting the hyphen in, it's part of
SourcePackageRelease.title - not sure why there is inconsistency. But
anyway, I've switched to an extra column.

>
> About the mockup (for further consideration):
> - It would be really cool if the "Add to blacklist" button was some new control that is a combination of a button and a drop down menu. Opening an extra dialog here seems too distracting to me.

Yes - to me also - it was my main concern with that reworking. I
chatted with Julian about this and we thought it makes sense to simply
provide two buttons "Blacklist Foo", or "Blacklist Foo 1.34-a"

What do you think?

>
> But with the points mentioned above included the screen shot of this stage of the page looks good to me. Thank you for doing such nice work! ;-)

Thanks for the thoughtful review!