docusaurus
docusaurus copied to clipboard
feat: improve syncing of user preferred theme
Pre-flight checklist
- [x] I have read the Contributing Guidelines on pull requests.
- [ ] If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
- [x] If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.
Motivation
As mentioned in #8074, I believe that if I change my preferred theme between sessions, I probably want it to update if my preferred theme has changed.
I implemented a feature that stores the user's preferred theme in localStorage (if respectPrefersColorScheme
is used). That way, we can check when the user visits the site the next time, if their preferred theme has changed.
This is needed because the theme
key (set when toggling using the switch) takes precedence over the media-matched prefers-color-scheme
entry. This means that if I use the switch, the site no longer respects my system preferences. This is good when navigating the page to keep it from switching themes when navigating. But if I come back later at night, and my system preference has changed, this change would previously not be recognized.
Let me explain the new behavior with some examples (examples goes both ways for light/dark mode):
-
The user browses the site during the day and uses light mode. When they come back at night, their system preferences has changed to dark mode. This change will be registered so the site will use dark mode too.
-
The user browses the site using light mode, because they don't like the dark mode of that site. The user always uses dark mode for their system preferences, so when coming back it won't have changed so the site will keep running in light mode.
-
The user navigates the site and uses the toggle to select light mode. They switch page. The dark mode system preference does not take precedence so we keep using light mode.
Test Plan
I have not yet added any tests, if you feel like it's required let me know and we can plan a test plan together.
Test links
- NaN
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
Closes #8074
Hi @juliusmarminge!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed
. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
[V2]
Built without sensitive environment variables
Name | Link |
---|---|
Latest commit | 81006e37eb778ccf0a63466ed86395375cc84bb5 |
Latest deploy log | https://app.netlify.com/sites/docusaurus-2/deploys/631ce91bb9f3440009d4e34b |
Deploy Preview | https://deploy-preview-8078--docusaurus-2.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
β‘οΈ Lighthouse report for the deploy preview of this PR
URL | Performance | Accessibility | Best Practices | SEO | PWA | Report |
---|---|---|---|---|---|---|
/ | π 81 | π’ 98 | π’ 100 | π’ 100 | π 80 | Report |
/docs/installation | π 78 | π’ 100 | π’ 100 | π’ 100 | π’ 90 | Report |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
I don't agree with that solution, as explained in https://github.com/facebook/docusaurus/issues/8074#issuecomment-1263635186
As a user, I want to visit the Docusaurus website only in dark-mode, despite using dynamic OS light/dark theme switches. It is legit to expect a site to not follow your OS theme changes, particularly if you want to have dynamic OS theme and you have per-site theme preferences that should override what your OS says.
The way it is designed is that a value in localstorage, if present, should always be applied.
If you want to reset to the OS theme you have to delete the localstorage value instead of trying to sync it to the os theme
Side note, your implementation does not take into account that the final choice of a theme must be decided in the critical path of the app, before the React hydration process even starts.
We have inlined JS here: https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-classic/src/index.ts#L29
Due to your implementation, the sync only happens later and it can produce a flash (ie is initially light and then switch to dark with your new sync system).
If we were to implement your solution (which I'm against), it should be inlined and <HTML data-theme>
should be set before React hydrates (ie not in useEffect()
).
My suggestion:
- You implement in this PR a solution to revert to OS setting
- Or we close this PR and someone else will do
Note we already have a case where a site initially has dark mode support, and later the site owner wants to remove it. You'll notice that we have code to handle that already, and it simply does setColorMode(null)
. If you want you can simply add a 3rd icon to the navbar switch that set the color mode to null, and it should work
@slorber If I never interacted with the color mode toggle, I won't consider the current mode "my choice"βit's merely the default, and I'm perfectly fine if it shifts.
@slorber If I never interacted with the color mode toggle, I won't consider the current mode "my choice"βit's merely the default, and I'm perfectly fine if it shifts.
I'm not sure what's the point you are trying to make here with this extra comment π but I agree with you.
Considering site has dark/light mode and respectPrefersColorScheme:
- If you never interact with the toggle, the localstorage value should be null and OS theme should apply (your comment?)
- If you ever interact with the toggle, the localstorage value should be filled and always be used in priority over OS theme
- there should be a way to move back to OS theme
Note: respectPrefersColorScheme
does not mean system OS will always win. We want users to be able to override and the override should not self-erase when the OS change theme
Do you suggest we make a 3-way-toggle with dark-light-system?
Would that toggle be opt-in?
Do you suggest we make a 3-way-toggle with dark-light-system?
Yes, I think we can give it a try where we have 3 distinct svgs to represent the 3 states. I'm pretty sure I've already seen some svg examples for "system". Otherwise we could use something similar to Mozilla:
![CleanShot 2022-10-07 at 11 27 36@2x](https://user-images.githubusercontent.com/749374/194521477-1140e485-6434-4d24-a38b-9d0876454586.png)
Would that toggle be opt-in?
We could make it opt-in for now and eventually turn it on by default if successful (with opt-out).
Note: if the user uses respectPrefersColorScheme: false
I guess it doesn't make sense because the color will never be the system theme even if user opt-infor having the 3rd option in the switch. We should find a way to prevent common mistakes and eventually emit warnings for options that do not play well together I guess.
Or maybe the 3rd option could be enabled automatically if respectPrefersColorScheme: true?
https://docusaurus.io/docs/api/themes/configuration#respectPrefersColorScheme
I noticed that we have this dual icons when JS is disabled on Docusaurus.
![CleanShot 2022-11-23 at 17 38 50@2x](https://user-images.githubusercontent.com/749374/203600900-7a3d2978-498b-44b2-94e7-5961de0146f4.png)
That probably makes sense to use something like that as the 3rd OS state?
Shouldn't be too hard to build, apparently just rendering both icons will lead to that result already π€ͺ
It's not perfect but better than nothing
closing this, as it's not the right direction to solve this problem
Let's continue the discussion in https://github.com/facebook/docusaurus/issues/8074