Implement toggle color scheme, automatic theme mode #1086
Conversation
Minor fix
There was a problem hiding this comment.
Thanks for submitting this PR!
Before I give a more thorough review, can anybody else repro the flash of content when switching themes? I'm on Chrome & macOS:
trimmed.mov
I think we'd have to resolve this bug before merging.
Separately - I do think changing the theme should persist across at least the current session (if not also cross-session), i.e. with local storage. I apologize for not making that clearer earlier when I outlined my PR comment. There should be ways to do this without causing a flash; React's new Beta docs do it quite seamlessly, for example.
|
I think I can confirm the flash on theme change. The search bar seems to be the most obvious thing. And I absolutely favor saving the selected theme in the localstorage. |
|
the way react docs beta uses is a bit ugly as it simply adds a css class to the body indicating the color scheme, I actually notice this problem too. for saving in the session, no problem I already have all the code ready (it's written in the documentation!), but I am quite sure that to comply with the GDPR it must be disabled. Moreover, saving in local storage does not convince me too much because of the annoying flashes, a completely different strategy would have to be devised. |
|
It's gdpr-compatible as long as you're using the localstorage. Cookies are bad since they're sent to the server with the page request. |
|
@max06 ok top! then I make it the default and try to fix everything |
|
Good luck fixing everything 🙈 |
|
I am reading that it is a very important problem https://css-tricks.com/flash-of-inaccurate-color-theme-fart/ |
I may be repeating things from the previous discussion, but I'm also fine with this approach - you do load up to double the styling, but in our case it's a relatively small stylesheet anyways and I'm willing to make this tradeoff for a relatively smooth experience. Then, you can use the media query value to inform whether or not the class should be applied. |
|
@mattxwang @max06 FART are fixed. The strategy used is as follows:
Since, I have added some settings to be able to customise this 100% (disable local storage, reverse or add themes to the switch button, change the FART time in case the server is slow ...). I have also documented everything.
I followed this idea, and then perfected it, there is also a second possibility of using css var (which theoretically is the best-practices, even if bootstrap fin bootstrap does not retain them so well and is only using them since version 5.X and with the possibility of disabling them), nevertheless I think this way is still very good and comparable, then if you prefer I implement it this way P.S. I had missed among the dozens of PR/issues this message... |
|
That looks much better now! There's a small bug in it: Starting without stored theme, it goes like - system-bright, system-dark, bright, dark, system-dark... the first detection is off. |
Excellent, I am happy!
Fixed in 40d3c5a |
|
Fix confirmed, thank you 🙇🏼 On first load I noticed a quick flash. It looked like two big purple buttons centered on the screen, maybe part of a mobile render. I'm just curious about it, no need to change anything. It happened so quick and only once. |
I cannot reproduce it...🙈 |
|
@simonebortolin I caught it! A bit lengthy. No cache, slowed network connection, no local storage. First theme switch after the initial load. Home._.Just.the.Docs.-.Vivaldi.2022-12-27.14-53-57.mp4 |
|
@max06 can you try to set |
|
Running with 1000 in my devcontainer, still visible. Not sure if it's a real problem though since you need a pretty slow connection to see it. |
If it still remains visible maybe that option is useless or not? in my tests it disappeared with slow connections |
|
Mhm... the light-theme is being loaded twice.
On first call, both themes are being loaded in parallel, both taking around 11 seconds. Changing the theme causes the light theme to be loaded again. The broken layout is visible during that change. |
Ummmh I don't operate on the cache... it should take it if it is already there. loading two themes in auto mode is correct. |
|
Okay, I've given it a bit more thought after some sleep and re-reviewing the conversation in #234. I'm realizing that, outside of the suggestion to base it off of PEP's three-way switch (which is nice!), there's actually no request to prefer a three-way switch over a two-way switch. Given that (in my opinion) the two-way switch is the de facto standard, I think we should do that for now. If we get more requests after pushing this feature out, I'm happy to revisit; however, this actually seems to be what the majority of users in that thread are requesting. To clarify, the two-way switch still resolves both my initial ask and the ask from users: if the setting is on I feel comfortable only providing out of the box functionality for the themes that we provide, and requesting custom theme authors to then bail out of our existing system and add their own code. We can provide snippets in the docs or simple instructions. I apologize that this has been a bit confusing / back-and-forth in this very long PR thread, but I'm of the opinion that this is the best way forward for now; it follows the general practice for most sites, is significantly easier to implement/understand, and still provides power users a way to customize. Given the current situation, I am happy to merge the above solution (essentially slightly slimming down this PR) and potentially addressing the FOUC/FART/etc in a follow-up. The flash is pretty jarring, and I'd like to resolve it before we make a formal release of this feature. What do we think? I would like to bias for action atm given how long this PR has gotten.
Followed up in the inline code comment! |
I honestly didn't understand anything: first the flash was a problem and had to be solved, now that it should be solved you have to revert the code and slimming down this PR? are 240 lines of new code and 90 lines removed, of which goodparte images or the like. I am quite confused. For my part it seems clear to me that there is no clear idea of what you want to do, because in #1086 (review) you talked that it was a problem now it seems that it is to be done in a second PR. You mean the flash when changing the theme for the first time? if you consider it to be a problem (and I believe it is not, since it only appears when changing the theme for the first time) it should be solved in this PR, as the only way to remove it is to use the light and dark classes and completely modify the JS, almost requiring starting from 0. now for me there are several things to do:
I would very much like just-the-docs to have a system for self-determination of themes, and I think that continuing to change my mind each time makes me give up continuing to discuss this PR and not want to throw my fixes in the main, because if I now have to revert to the first commit it was all wasted time. Finally, I still don't know whether you prefer this code or #1099 |
|
Hm, I apologize if I'm not being clear. Let me try to respond to each of your questions concisely.
No, I'm not saying you need to revert your existing work (in this case). What you've done is good! Perhaps the confusion is on the overloading of the word "flash". There is:
By my comment, I was mostly referring to the latter. Sorry if that wasn't clear.
I'm not sure what "goodparte images or the like" is, but to directly address the linked review - I'm now saying, it's okay if we don't resolve that problem in this PR. I'm trying to be more lenient so you need to do less work to merge this PR. If you personally still want to resolve this in this PR, that's fine by me too; just not a requirement to make things easier.
I'm not sure if the latter assessment is true (in particular, there's probably quite a bit of code splitting we can do - the most jarring part of the flash is various unstyled components of the page). I still want to make this specific PR a relatively small / non-style change. Relaxing that requirement down the line gives us quite a bit more creativity to resolve other problems.
I agree we're asking you to do a lot here. This is the unfortunate nature of an ambitious feature that we hadn't scoped out well enough before implementing. Part of this is certainly on me for not providing the clearest guidance; however, it was a bit tricky for me given some travel nightmares and the general holiday season. In the future, I also suggest that we scope out more of this in an issue, before we submit a PR.
My hope is that this is only changing a few lines of code. You just need to remove the existing icon + cycle element for
Maybe I can be more clear. I'm looking to replicate the general experience found in sites like the React beta docs, Stylelint's example of a Docusaurus website, etc. To enumerate the logic precisely, on page load:
On theme toggle, change the theme to the newly-selected one. If the site has enabled Hopefully that makes things clearer. To me, this is both a standard design flow, and only a handful of lines of code to check conditions. I'm not sure what you mean by
Nope, not saying that.
Sounds good.
Again, not saying you need to revert your commits. I also understand that you are looking for more power in this feature than just light and dark mode. My main point of the previous comment is, for this PR, let's just focus on supporting light and dark mode out of the box. Developers who want more functionality can take an escape hatch to implement a three-way switch, for example.
Let's stick with this approach right now. |
|
@mattxwang There now you have been clear and concise. Very good, now I know what to do.
I maybe liked it better. I would like to get a definitive answer on this, though. That's what I wanted to know:
Yes! and I fully agree with the workflow, but I would take the liberty of not saving the theme in localStorage if it has not been changed.
simple: instead of saving light and dark I just save 'not-auto', i.e. I load the reverse to the |
|
I have finished goodapart of the university commitments, I may come back to work on this PR |
|
Sounds good - any idea on the timeline for this? For transparency's sake, I plan on doing a big chunk of dev work on JtD over the next two weeks (before my teaching load starts), which I may (or may not) devote to forking this PR (and/or another color scheme one) and merging it in. |
|
As I understand it, you opened PR #1328 which is mainly mine and the things that are missing are listed there right? So I better upgrade this PR to the There are still a few small things left on the documentation and accessibility that I think you'll be able to help me solve the annoying flashes. I would also like to complete this PR because honestly this mechanism came out relatively well and I would like it to become quite a defacto at least in the systems I create |
|
@simonebortolin so far, #1328 is just resolving the merge conflicts and some minor bugs. Feel free to update this one if you'd prefer, or base off of the branch; I have no preference. (my earlier question was more on timeline - do you think you'll be working on this now?) |
yes, I have some free time, not too much, but enough to finish this PR, I think in 2 weeks obviously you first need to understand what there is to work on |
|
Now this PR is synced with #1328 |
simonebortolin
force-pushed
the
yet-another-dark-switch
branch
from
bbb8eb0
to
4c37a58
Compare
|
Then it seems to work fine for me locally and in my example site. I moved the CSS introduced by PR #1244 inside the Same thing I moved the expand all immediately after the html so there is no annoying flash I added a fallback mode in case the js is missing and the local storage theme is enabled TODO:
|
I had actually considered using
That seemed to motivate using JS for disabling. But I'm not an expert on HTML, and I also have very little experience with JS. |
|
It seems to me that the standard is talking about another situation completely opposite: that is, putting buttons in a forum always and not relying on the noscript tag, due to the fact that if, for example, there is no ontextchange event on a browser you can always calculate the area of the triangle. Which is a best practice, present for example on Google that although you can select on autocomplete the search button does not disappear I honestly find it silly to disable css like that when always the standard says that the noscript tag was made for that (see https://html.spec.whatwg.org/multipage/scripting.html#the-noscript-element) which just talks about styles in the head with noscript |
|
Hi, I'm new to Just the Docs. The trick is to define themes as classes. .theme-default {
{% include css/just-the-docs.scss.liquid color_scheme=color_scheme %}
}
.theme-dark {
{% include css/just-the-docs.scss.liquid color_scheme="dark" %}
}jtd.setTheme = function(theme) {
let old_theme = this.getTheme();
if (theme != old_theme) {
localStorage.setItem('theme', theme);
document.documentElement.classList.add('theme-' + theme);
document.documentElement.classList.remove('theme-' + old_theme);
}
}You can try my toggle button to see there is no flashes on firefox. I'm too novice at jekyll to generalize the method. |
|
Thanks for commenting @matyalatte! To answer
I'm sure you could (without looking too closely at it). In general, I think homebrewing your own automatic theme adjustment is not too challenging with Just the Docs, given a specific use-case (and I'm glad that you figured it out!); part of the challenge with the long set of issues/PRs has been driving consensus across all of our users about what the best path forward has been. I have really been struggling to find OSS time as of late, but eventually ™️ I will be able to personally drive home most of these feature requests, particularly when the summer rolls around. In the meantime, would love to see more action from others if they have the bandwidth! |
|
Thanks for the reply!
It's ok. I know how hard it is to maintain OSS. I can wait and use my custom theme for now. |
|
I had already sketched out a theme with your strategy but complications arose and the recent changes to JTD that I dosent like will make this difficult to implement, however if you need help I am there |
Yes yet another implementation for the dark mode toogle (#234):
This implementation use:
scriptinheadfor load the css from local storage instead of loading them statically and then changing themcustom.htmlfile for icon, this file can serve for add a custom icon for theme or other purposes, for example in the https://hack-gpon.github.io/ (that uses my fork that implementing this PR and others - see: https://github.com/hack-gpon/hack-gpon.github.io/blob/main/_includes/icons/custom.html - site now uses all the PRThis PR is inspired by existing PRs such as #302, #464, #560, #858 and #966
This PR use:
In particular as requested by #234 (comment)
yes! use the
prefers-color-schemeon<link>, allows default values and has no regression problems.yes! there is a user-side implementation, with a hint of how to implement it in local storage, and a config-side one
_config.ymlyes! everything is documented
is possible by disabling the flag and use the suggestion that ared inside the documentation
A long description of the PR and why I implemented it this way:
N.B. for @mattxwang I also put the icon licence in the same style as https://github.com/just-the-docs/just-the-docs/blob/main/_includes/icons/external_link.html and #945
a complete description of why the fart dosen't apply on the loading page are in https://tpiros.dev/blog/three-state-light-dark-theme-switch/