Skip to content
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: improve syncing of user preferred theme #8078

Closed
wants to merge 2 commits into from

Conversation

juliusmarminge
Copy link

@juliusmarminge juliusmarminge commented Sep 10, 2022

Pre-flight checklist

  • 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.
  • 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-8078--docusaurus-2.netlify.app/

Related issues/PRs

Closes #8074

@facebook-github-bot
Copy link
Contributor

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!

@netlify
Copy link

netlify bot commented Sep 10, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 81006e3
🔍 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.

@github-actions
Copy link

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

@facebook-github-bot
Copy link
Contributor

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 facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 10, 2022
@slorber
Copy link
Collaborator

slorber commented Sep 30, 2022

I don't agree with that solution, as explained in #8074 (comment)

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 slorber marked this pull request as draft September 30, 2022 14:36
@Josh-Cena
Copy link
Collaborator

@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
Copy link
Collaborator

slorber commented Sep 30, 2022

@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

@juliusmarminge
Copy link
Author

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

Would that toggle be opt-in?

@slorber
Copy link
Collaborator

slorber commented Oct 7, 2022

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
Copy link
Collaborator

slorber commented Nov 23, 2022

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
Copy link
Collaborator

slorber commented Dec 8, 2022

closing this, as it's not the right direction to solve this problem

Let's continue the discussion in #8074

@slorber slorber closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reset to OS theme / colorMode should possible
4 participants