-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix white flashes on dark mode #554
Conversation
@msbt Thank you! |
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.
Hi. Thanks a stack for the fix and corresponding improvements, also adding one more iteration on bringing the code base closer to upstream's Furo.
The updates to base.html
and page.html
are quite extensive, but I am trusting you to do the right things there. Let's test it diligently on behalf of another -dev release and a few downstream projects, and it will be all right, I guess.
<style> | ||
body { | ||
{{ declare_css_variables(theme_light_css_variables, furo_pygments.light) }} | ||
--color-code-background: #f8f8f8; | ||
--color-code-foreground: black; | ||
} | ||
|
||
@media not print { | ||
body[data-theme="dark"] { | ||
{{ declare_css_variables(theme_dark_css_variables, furo_pygments.dark) }} | ||
--color-code-background: #202020; | ||
--color-code-foreground: #d0d0d0; | ||
} | ||
@media (prefers-color-scheme: dark) { | ||
body:not([data-theme="light"]) { | ||
{{ declare_css_variables(theme_dark_css_variables, furo_pygments.dark) }} | ||
--color-code-background: #202020; | ||
--color-code-foreground: #d0d0d0; | ||
} | ||
} | ||
} | ||
|
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.
It looks like variables have been used before here, but now you are using hard-coded values. Is it no longer possible to use variables going forward?
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.
If it is no longer possible, can we still define the colors in a different location, for example in a custom.scss
, if we have it already, instead of within an .html
file?
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.
@amotl Well the thing is, those variables only work if we import a whole lot more config stuff. This file originates from https://github.com/pradyunsg/furo/blob/main/src/furo/theme/furo/partials/_head_css_variables.html and only work if we include the bulk of https://github.com/pradyunsg/furo/blob/main/src/furo/__init__.py - which I don't have the resources to look at these days. So adding thise two variables manually was the next best thing until one of those cold winter nights arrive to do it ;)
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.
Thank you for clarifying!
//@import "colors" | ||
@import "colors" |
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.
Wow, excellent! 💯 Kudos for converging the code base to be even closer to Furo, now also importing its SASS- or SCSS-based color definitions.
Summary of the changes / Why this is an improvement
After introducing dark mode, we recognized white flashes when navigating between pages. This patch should fix it. It wasn't trivial, since some of the logic was custom and some came from furo. I ended up migrating more bits from furo in our template and also updated our
page.html
andbase.html
to make it more compatible.Quite a bit has changed, so this needs to have proper review again before releasing wide (e.g. canonical links)
Demo is here: https://crate-docs-theme--554.org.readthedocs.build/en/554/
cc @geragray @kneth