fix(theme-common): do not persist color mode for OS-triggered changes #7200
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-7200--docusaurus-2.netlify.app/ |
|
Size Change: +288 B (0%) Total Size: 800 kB
ℹ️ View Unchanged
|
b1c170f to
dc1186f
Compare
72b7526 to
258e521
Compare
258e521 to
ca463e9
Compare
| setColorMode(matches ? ColorModes.dark : ColorModes.light); | ||
| // Do not persist this change because it's system-theme-triggered and not | ||
| // a user action. | ||
| setColorMode(matches ? ColorModes.dark : ColorModes.light, { |
There was a problem hiding this comment.
this is not good enough.
Even if persisted value is not overridden by theme changes (and thus appropriately restored on OS theme change), switching theme in the OS is still applied immediately.
- Select dark mode with toggle
- Move OS to light mode => UI changes to light mode (it shouldn't)
- Refresh => UI is restored to dark
There was a problem hiding this comment.
I think it should change to light mode. I don't really know, but what's the best UX? I feel like if OS is light mode, the site should really reflect that.
There was a problem hiding this comment.
We have 2 possible choices here. We may or not agree on the best choice but at least it should be consistent: if you refresh a page you should end up with the same color as before, in all cases
There was a problem hiding this comment.
Yes, I've set it to deleting the storage (using setColorMode(null)). This should force it to reset to system settings.
| }, [disableSwitch]); | ||
|
|
||
| const setColorMode = useCallback( | ||
| (newColorMode: ColorMode | null, options: {persist?: boolean} = {}) => { |
There was a problem hiding this comment.
Now the persist option is actually unused internally. Keeping it might still be useful? I don't have a great use-case for it though, given that it would be reset on refresh.
There was a problem hiding this comment.
See my comment here #7200 (comment)
It's up to you, I'm fine with both solutions as long as it's consistent
| if (window.matchMedia('print').matches || previousMediaIsPrint.current) { | ||
| previousMediaIsPrint.current = window.matchMedia('print').matches; | ||
| return; | ||
| } | ||
| setColorMode(matches ? ColorModes.dark : ColorModes.light); | ||
| setColorMode(null); |
There was a problem hiding this comment.
🤷♂️ this wouldn't be my choice but I'm ok with that
I would have used something like:
const storedColorMode = ColorModeStorage.get();
if (storedColorMode === null) {
setColorMode(matches ? ColorModes.dark : ColorModes.light,{persist: false});
}There was a problem hiding this comment.
I don't really know. I'm slightly leaning towards "OS setting should override user choice" until we have a dedicated "OS default" theme option. The localstorage is really just a convenient way to restore what was previously there the next time the user opens the site; its semantic should not be "freeze the color mode until the user explicitly clicks the button"
|
Yeah, I feel like this is more extensible. |
|
Note the public api (+ doc) is not updated: readonly setColorMode: (colorMode: ColorMode) => void;Should we make this officially public or kept as a secret API? 🤷♂️ API seems good enough to me to become public
I guess it's fine, users are unlikely to use this option without knowing what they are doing |
|
Let's keep it secret for now. I don't see a great use-case for it, and without actual warning of its weird UX + a concrete example use-case I'd rather not let them do it. |
Motivation
Fix #7199.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan