launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Please set a commit message for your merge proposal; the landing machinery requires this.  Also, it's a good idea with UI branches to give some example pages that highlight your change.  In the absence of one I looked at https://code.launchpad.dev/gnome-terminal, but you may have a better example.

This doesn't seem to quite do what you want.  The way that descendant selectors work mean that what you're doing here is selecting a tr element inside another tr.  This works better:

  table.listing tbody tr:not(.thead):hover

But not(.thead) is weird inside tbody; and in any case tables aren't obliged to have an explicit tbody element.  Perhaps what you meant is more like this?

  table.listing tr:not(.thead):hover

The contrast between white and lightgrey feels a bit too sharp and jarring to me, and I wonder if there's a shade in between those two that we could pick, preferably one already in use in Launchpad.  For selected rows in the official tags form, we use #eee, and that feels much nicer to me here.

There are some views where this looks distinctly weird.  For example, in a development instance with sampledata, try https://launchpad.dev/ubuntu/+source/evolution and hover over the various rows in the table at the bottom.  Can we find a way not to highlight the blank rows?
-- 
https://code.launchpad.net/~rbanffy/launchpad/highlight_listing_tr/+merge/258766
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.