Code review comment for lp:~rbanffy/launchpad/highlight_listing_tr

Revision history for this message
Colin Watson (cjwatson) wrote :

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?

review: Needs Fixing

« Back to merge proposal