-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: improve text readability and webpage performance #3132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial points on some CSS changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
My main issue is with the high contrast of dark mode (look at this site, GitHub.com, as an example of the contrast between background and text that I prefer), but I don't think that's within scope for this PR. |
We changed it from the original greenish |
To my eyes both your colours look black! 😄 |
From the rationale in the PR:
I agree with this especially on the fonts point. I do have a slight hesitation on whitespace though, perhaps as evidenced by the side-by-side below. I think there is value in some level of information density, and e.g. the new change means that the PEPs table is 'below the fold' as there is significantly more space above and below headings, paragraphs, and lists. Perhaps there's a compromise to increase our margins and padding somewhat, but not to the level on the right hand side of the screenshot below? A |
Had the same feedback about content density! |
Keeping the whitespace reduction around the headers is likely OK. We get most of the benefits from the font change and line height tweaks, and leaving the headings tighter to increase content density seems reasonable to me! |
We can circle back about the vertical spacing once there's more eyes on the new style and more feedback. Personally, I like the change in the PR. I think it's big enough to make it easier to follow reading line by line, but small enough to retain sensible information density. |
Space is important for the readability of any kind of text.
Right now the PEP webpages use a Google font with a lot of custom CSS to pull the text in tighter and with less whitespace.
I find the font, small size and tight spacing hard to read (and my eyes aren't getting any younger!). Using default fonts and spacing feels more readable and loads faster. The Google font causes an annoying initial layout shift after it's loaded and the page is re-rendered.
Readability was brought up when we overhauled the PEPs infra as part of PEP 676. But as the other improvements and benefits of migration were so big, it made sense to press ahead without delaying to discuss this further.
One year on, the overhauled infra has been working really well. Let's now improve the readability :)
Original comments
Some of the comments from the original discussion:
This PR
This PR uses a default "system font stack", and removes a lot of the custom CSS to use default CSS spacing. Let's compare:
Original
PR
Benefits according to https://systemfontstack.com:
Many sites use a system font stack: GitHub, Stack Overflow, Booking.com, and Weather.com, Bootstrap, Medium, Ghost, PubMed, and the WordPress dashboard. As do the Furo (used on the devguide) and pydata-sphinx-theme themes. See more on the history and rationale.
I looked at sites such as Mozilla's MDN Web Docs (for example: https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching) to compare CSS. Their site is technical writing which I find clearly laid out, with space to breathe. They use a lot of default CSS values.
I've been using this preview build to read recent PEPs and found it much clearer.
In numbers
Running some numbers on Google's Lighthouse tool (ignore the SEO drop on the PR, that's because RtD rightly hides preview builds via robots.txt) shows better performance with the PR:
Note:
And: