8000 fix(theme-common): do not persist color mode for OS-triggered changes by Josh-Cena · Pull Request #7200 · facebook/docusaurus · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(theme-common): do not persist color mode for OS-triggered changes #7200

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 21, 2022
Merged

fix(theme-common): do not persist color mode for OS-triggered changes #7200

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc1186f
fix(theme-common): do not persist color mode if switch is disabled
Josh-Cena Apr 18, 2022
ca463e9
New setColorMode(null) API
Josh-Cena Apr 21, 2022
4bc9c27
reset to default without RPCS
Josh-Cena Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 38 additions & 15 deletions packages/docusaurus-theme-common/src/contexts/colorMode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,39 @@ function useContextValue(): ContextValue {
getInitialColorMode(defaultMode),
);

const setColorMode = useCallback((newColorMode: ColorMode) => {
setColorModeState(newColorMode);
storeColorMode(newColorMode);
}, []);
useEffect(() => {
// A site is deployed without disableSwitch
// => User visits the site and has a persisted value
// => Site later enabled disableSwitch
// => Clear the previously stored value to apply the site's setting
if (disableSwitch) {
ColorModeStorage.del();
}
}, [disableSwitch]);

const setColorMode = useCallback(
(newColorMode: ColorMode | null, options: {persist?: boolean} = {}) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment here #7200 (comment)

It's up to you, I'm fine with both solutions as long as it's consistent

const {persist = true} = options;
if (newColorMode) {
setColorModeState(newColorMode);
if (persist) {
storeColorMode(newColorMode);
}
} else {
if (respectPrefersColorScheme) {
setColorModeState(
window.matchMedia('(prefers-color-scheme: dark)').matches
? ColorModes.dark
: ColorModes.light,
);
} else {
setColorModeState(defaultMode);
}
ColorModeStorage.del();
}
},
[respectPrefersColorScheme, defaultMode],
);

useEffect(() => {
document.documentElement.setAttribute(
Expand All @@ -85,13 +114,9 @@ function useContextValue(): ContextValue {
if (e.key !== ColorModeStorageKey) {
return;
}
try {
const storedColorMode = ColorModeStorage.get();
if (storedColorMode !== null) {
setColorMode(coerceToColorMode(storedColorMode));
}
} catch (err) {
console.error(err);
const storedColorMode = ColorModeStorage.get();
if (storedColorMode !== null) {
setColorMode(coerceToColorMode(storedColorMode));
}
};
window.addEventListener('storage', onChange);
Expand All @@ -109,12 +134,12 @@ function useContextValue(): ContextValue {
return undefined;
}
const mql = window.matchMedia('(prefers-color-scheme: dark)');
const class="x x-first x-last">{matches}: MediaQueryListEvent) => {
const => {
if (window.matchMedia('print').matches || previousMediaIsPrint.current) {
previousMediaIsPrint.current = window.matchMedia('print').matches;
return;
}
setColorMode(m 8000 atches ? ColorModes.dark : ColorModes.light);
setColorMode(null);
Copy link
Collaborator
@slorber slorber Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️ 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});
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

};
mql.addListener(onChange);
return () => mql.removeListener(onChange);
Expand All @@ -139,7 +164,6 @@ function useContextValue(): ContextValue {
);
}
setColorMo 643A de(ColorModes.light);
storeColorMode(ColorModes.light);
},
setDarkTheme() {
if (process.env.NODE_ENV === 'development') {
Expand All @@ -148,7 +172,6 @@ function useContextValue(): ContextValue {
);
}
setColorMode(ColorModes.dark);
storeColorMode(ColorModes.dark);
},
}),
[colorMode, setColorMode],
Expand Down
0