-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: support automatic selectable dark theme #560
base: main
Are you sure you want to change the base?
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.
@henryiii thanks for this PR. See my next comment for some issues that I've noticed.
I guess the only reason the build checks are currently failing is that just-the-docs.gemspec
is outdated – the bundler version constraint needs to be relaxed to:
spec.add_development_dependency "bundler", ">= 2.1.4"
@henryiii I'm now testing henryiii:feat/autodark locally, using Jekyll 3.8.7. With
if (window.matchMedia) {
window.matchMedia('(prefers-color-scheme: dark)')
.addEventListener('change', event => {
if (event.matches) {
jtd.setTheme('nil');
} else {
jtd.setTheme('nil');
}
});
if(window.matchMedia('(prefers-color-scheme: dark)').matches) {
jtd.setTheme('nil');
}
} It appears that Setting Can you reproduce the above locally? I guess Liquid tests can be added to fix the issue with the |
I'm looking into it. Ah, "nil" is not a dedicated true/false value in YAML, so it't the string nil. That's easy to fix. Will look into the loading next. |
Fixed the handling of nil. Firefox is odd - it seems to be trying to show a partially loaded page for some reason (the flashed page is not complete). Chrome and Safari are fine. We could move this up in to the onReady function, perhaps, but I don't think that would fix the flashing? I couldn't get the syntax quite right for accessing |
Looks like Firefox is rather known for the white flash when loading. https://www.reddit.com/r/firefox/comments/8yqhu8/how_to_change_white_flash_when_loading_webpage/ https://support.mozilla.org/en-US/questions/1269392 |
Thanks for fixing the handling of That Firefox flashing is very annoying! I tried moving the new code inside the onReady function, but it didn't seem to make any difference. With PR #464, Firefox does not flash at all when navigating between pages in dark mode. I guess that is because the CSS already includes the code for the dark mode, so Firefox doesn't need to reload it? But I have no idea how to combine that technique with the use of |
For me, navigating the pages of https://scikit-hep.org on Firefox, the only flashing I get is for the initial page. After that, it is stable. I think firefox is doing some sort of fast-load trickery to make the page appear to load quicker, but it's interfering with this. There might be a workaround, but not sure what it would be. The SCSS method didn't seem bad, ether. |
Indeed: browsing https://scikit-hep.org with Firefox, I saw flashes the first time I visited a few pages, but none appeared after that. When I browse a local copy of https://github.com/scikit-hep/scikit-hep.github.io, however, I see a flash in Firefox whenever I navigate to a different page, or reload the current page. The flash is a bit quicker when a page is revisited, but it is still quite annoying. Similarly, when I visited https://pmarsceill.github.io/just-the-docs/docs/customization/#color-schemes with Firefox, clicking on
Repeating the above sequence produces the same flashes. I suspect |
Hello, I was developing exactly the same thing when I came across this PR. What's new about a merge of this PR ? |
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.
Can you squash your fix commit into one.
6d3c93a
to
03755f6
Compare
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.
Thanks for the update to the setting in _config.yml
. The updated preview works without any FOUCs in Firefox and Safari on macOS, so that issue has been resolved.
I have very little experience of JS programming, so I'm leaving it to the other maintainers to review that file.
I think it would be helpful to copy the explanation of dark_color_scheme
from customization to configuration.
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.
Going to leave a comment saying - on a first skim, this looks great, and I think I'll probably merge this one (opposed to #966). I'll need to do a bit more thinking over the next few days and will either leave nit comments or approve it.
Thanks for your work on this @henryiii (and your other contributions), they're much appreciated!
As an aside, I do also want to add a toggle for dark mode. Breaking it up into multiple PRs (even spread across multiple releases) is fine by me.
I now read this message and do not understand why it is preferable to load a theme via js than via media query (#858 and #966).... this causes flashes when starting the browser as documented at the beginning of the PR, it is absolutely preferable to use media queries: JS code:
vs pure HTML5 code:
I would like to add that a fix will most probably be added to this PR as well, so that the buttons can be used to change the theme (code in #966 but not #858). Yes it is always possible to cherry-pick, but does it make sense to add all this JS code?
I have create a #1086 that even merge part of this PR and other PR: namely the fact that you can enable or disable this from _config.yml. |
Not only when starting it: I see really bad FOUCs also when navigating between the deploy preview pages with Firefox (108.0.1, macOS 12.6.2, MacBook Pro M1, system state auto/dark). |
I need to fix
gh
on this computer, it opens mvim and then submits the PR without waiting for this...Anyway, this closes #234 by adding a
dark_color_scheme
parameter that enables auto-switching to whatever it is set to if it is set.Not really tested yet.I've used this on https://scikit-hep.org, but have not tested the configuration part.