docusaurus icon indicating copy to clipboard operation
docusaurus copied to clipboard

feat: improve syncing of user preferred theme

Open juliusmarminge opened this issue 1 year ago β€’ 4 comments

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):

  1. 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.

  2. 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.

  3. 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

juliusmarminge avatar Sep 10 '22 19:09 juliusmarminge

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!

facebook-github-bot avatar Sep 10 '22 19:09 facebook-github-bot

[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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Sep 10 '22 19:09 netlify[bot]

⚑️ 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

github-actions[bot] avatar Sep 10 '22 19:09 github-actions[bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot avatar Sep 10 '22 20:09 facebook-github-bot

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 avatar Sep 30 '22 14:09 slorber

@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.

Josh-Cena avatar Sep 30 '22 16:09 Josh-Cena

@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

slorber avatar Sep 30 '22 16:09 slorber

Do you suggest we make a 3-way-toggle with dark-light-system?

Would that toggle be opt-in?

juliusmarminge avatar Oct 06 '22 17:10 juliusmarminge

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

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

slorber avatar Oct 07 '22 09:10 slorber

I noticed that we have this dual icons when JS is disabled on Docusaurus.

CleanShot 2022-11-23 at 17 38 50@2x

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

slorber avatar Nov 23 '22 16:11 slorber

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

slorber avatar Dec 08 '22 12:12 slorber