Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style: Fix colors and style for all admonitions #2691

Merged
merged 4 commits into from
Jul 6, 2022

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Jun 29, 2022

Since the basic admonition admonition styles are just applied to a couple individual admonitions, rather than admonitions in general, those other than "warning" and "note" won't display as admonitions at all. In addition to fixing that, this PR adds appropriate, standard colors for each of the additional admonition types in both light and dark mode.

Discovered in and required by #2690 .

image

Slightly outdated screenshots also showing status quo

image

image

@CAM-Gerlach CAM-Gerlach added enhancement infra Core infrastructure for building and rendering PEPs labels Jun 29, 2022
@CAM-Gerlach CAM-Gerlach requested a review from hugovk June 29, 2022 06:33
@CAM-Gerlach CAM-Gerlach self-assigned this Jun 29, 2022
@hugovk
Copy link
Member

hugovk commented Jun 29, 2022

Accessibility:

In dark and light mode, the link colour on the warning background has low contrast:

image

image

And also in light mode for the link colour on the error background:

image

Can we increase the ratio?

@CAM-Gerlach
Copy link
Member Author

I first took a look at tweaking the link color; in light code I was able to darken it a little to #006699 from #0072aa, which helps across the board, though I didn't want to go too much darker to avoid contrast problems with the regular text. In dark mode, however, the link color is already pretty light, to the point where increasing it any more would make it hard to distinguish from normal text, and users have already mentioned that it is bright/high contrast enough to fatigue their vision, so I didn't push it any further for now.

In dark and light mode, the link colour on the warning background has low contrast

I darkened the dark mode color to #804000 from #884400, which gets the contrast ratio up to 4.00 from 3.68. The problem with going any darker (or even to some extent that dark, and is why I set it that high in the first place) makes the color brown instead of orange, which doesn't communicate the desired meaning, as well as reducing contrast with the background.

Likewise, I lightened the light mode color to #ffddbb from #ffccaa, which increases contrast to 4.85 (with the link change) vs 3.6. Likewise, making this any lighter will not give the admonition the appropriate semantic meaning/priority and significantly impair contrast with the background, making it hard to spot at a glance especially for said users who need the higher contrast.

And also in light mode for the link colour on the error background

I lightened the light mode color from #ffaaaa to #ffcccc, which combined with the link change raises contrast to 4.4 from 2.9. Again, any lighter and it won't visually communicate the gravity of the message, and will also be difficult or impossible to spot due to low contrast against the background.

Also, I bumped the light mode attention/important color from #bbccff to #bbddff, which increases contrast from 3.3 to 4.5.

The others I checked all were at to well above these latter levels, especially with the across the board link change.

@hugovk
Copy link
Member

hugovk commented Jun 30, 2022

Thanks!

We don't have to only adjust the "global" link colour, we can tailor it specifically for each admonition.

The problem with going any darker (or even to some extent that dark, and is why I set it that high in the first place) makes the color brown instead of orange, which doesn't communicate the desired meaning, as well as reducing contrast with the background.

Yeah... One option with orange background is to have dark foreground text, then we can get a bright orange:

image

Or this looks like a a good tool for finding contrast (with a guide here), you give it a foreground and background colours and tell it which one to edit, and it can give you a range of options.

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Jun 30, 2022

I made some further tweaks, making the link color higher contrast inside annotations only, while using some of the additional leeway to re-adjust the admonition colors to a more optimal sequence and modestly higher contrast from the background and each other, while still preserving at least a 4.5 contrast ratio between all of them (including, now, orange on the dark BG).

Updated screenshot (also in OP)

image

We don't have to only adjust the "global" link colour, we can tailor it specifically for each admonition.

Yeah, I thought about that before, but didn't go for it at the time since it would require more theme variables, would still reduce the contrast between links and normal text, and it could look a little inconsistent. But that is probably the simplest and least impactful solution and less sub-optimal the shifting the BG colors more, so I went with it.

Yeah... One option with orange background is to have dark foreground text, then we can get a bright orange:

We could do that, but we'd have to switch all the other colors that might appear in the warning too, which is pretty non-trivial, and the background would have to be very bright compared to rest of the page and the other admonitions; we'd have to make at least error/danger do that as well to not have warning outshine it.

Or this looks like a a good tool for finding contrast

Great tool, thanks! Very handy.

@CAM-Gerlach CAM-Gerlach changed the title Style: Fix basic style for other admonitions Style: Fix colors and style for all admonitions Jun 30, 2022
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@CAM-Gerlach
Copy link
Member Author

If no one has any objections, I'll merge this in a ≈day so #2690 is ready to go whenever it gets merged.

@CAM-Gerlach
Copy link
Member Author

Going ahead and merging this so I can update #2690 with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement infra Core infrastructure for building and rendering PEPs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants