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

Infra: Only invert image brightness, not hue #2949

Merged
merged 1 commit into from Jan 6, 2023

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Jan 6, 2023

In #2409, CSS was added to invert images that would otherwise be hard to see on dark background.

Since inversion doesn’t just affect brightness, but also hue, there’s a problem: Some background colors have perceptually less contrast with their text when hue rotated.

This PR rotates their hue back, fixing this. Also I prefer the less vaporware aesthetic of the original colors. E.g. the white-on-red is much more readable than white-on-cyan in PEP 458:

Without this PR With this PR
without with

@Rosuav
Copy link
Contributor

Rosuav commented Jan 6, 2023

Yyyyyyeeah that's a big improvement 🤣 Looks good.

@flying-sheep
Copy link
Contributor Author

Thanks! The red used in light mode is a bit dark, so contrast isn’t ideal to start with, but that cyan really makes it much worse.

@CAM-Gerlach CAM-Gerlach added the infra Core infrastructure for building and rendering PEPs label Jan 6, 2023
@CAM-Gerlach CAM-Gerlach changed the title Only invert image brightness, not hue Infra: Only invert image brightness, not hue Jan 6, 2023
@AA-Turner AA-Turner merged commit 631ee6c into python:main Jan 6, 2023
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Jan 6, 2023

Seems like a great change overall, thanks. However, even a cursory check of the render previews revealed a serious problem, albeit not directly due to the changes in this PR.

Since the last build yesterday, Sphinx jumped 3 (!) versions in one day (from 6.0.0 to 6.1.1, the latter of which isn't even documented in the Sphinx changelog linked in the 6.1.1 release, which seems to be because the change wasn't yet merged/built for master), and something appears to have broken as no images at all are being copied during the build (checked build output and the preview site) so none of the images work. If I were to guess, of the items listed in the changelog, sphinx-doc/sphinx#10944 seemed most likely to be the culprit, with nothing else really sticking out. Merging this (or any other PR that triggers a rebuild) will thus break our production site, and all render previews are broken (so we cannot actually confirm that this change renders as intended on CI).

On a related note, I've learned the hard way to always spend a few seconds checking the render preview before approving or merging, which it appears no one did 🙂

EDIT: ...Aaaand it's broken now. I'll rush through a PR to confirm the exact version affected and upper-cap it to one before, and hopefully it can be fixed upstream.

Ironically, the very first build it affected was that of this PR which was all about modifying the display of images; while initially that made the investigation take much longer, without that it may have been a while before one of us noticed (as isn't obvious the images are missing that should be there unless you are on PEPs that have then and specifically are looking for them).

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

Successfully merging this pull request may close these issues.

None yet

5 participants