-
Notifications
You must be signed in to change notification settings - Fork 28
fix(theming): allow three states in useTheme #656
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,49 +1,112 @@ | ||
| import { useState, useEffect, useCallback } from 'react'; | ||
|
|
||
| const THEME_STORAGE_KEY = 'theme'; | ||
| const THEME_PREFERENCES = new Set(['system', 'light', 'dark']); | ||
|
|
||
| /** | ||
| * Sets up theme toggle button and system preference listener | ||
| */ | ||
| const getSystemTheme = () => | ||
| matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; | ||
|
|
||
| /** | ||
| * Applies the given theme to the `<html>` element's `data-theme` attribute | ||
| * and persists the theme preference in `localStorage`. | ||
| * Retrieves the stored theme preference from local storage. | ||
| * | ||
| * @param {string} theme - The theme to apply ('light' or 'dark'). | ||
| * @returns {'system'|'light'|'dark'|null} The stored theme preference or null if not found. | ||
| */ | ||
| const applyTheme = theme => { | ||
| document.documentElement.setAttribute('data-theme', theme); | ||
| document.documentElement.style.colorScheme = theme; | ||
| localStorage.setItem('theme', theme); | ||
| const getStoredThemePreference = () => { | ||
| try { | ||
| const storedTheme = localStorage.getItem(THEME_STORAGE_KEY); | ||
| return THEME_PREFERENCES.has(storedTheme) ? storedTheme : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * A React hook for managing the application's light/dark theme. | ||
| * Stores the theme preference in local storage. | ||
| * If storage is unavailable, it fails silently, allowing the application to continue functioning with an in-memory preference. | ||
| * | ||
| * @param {'system'|'light'|'dark'} themePreference - The theme preference to store. | ||
| */ | ||
| const setStoredThemePreference = themePreference => { | ||
| try { | ||
| localStorage.setItem(THEME_STORAGE_KEY, themePreference); | ||
| } catch { | ||
| // Ignore storage failures and keep non-persistent in-memory preference. | ||
avivkeller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Applies a theme preference to the document. | ||
| * | ||
| * The persisted preference can be 'system', but the applied document theme is | ||
| * always resolved to either 'light' or 'dark'. | ||
| * | ||
| * @param {'system'|'light'|'dark'} themePreference - Theme preference. | ||
| */ | ||
| const applyThemePreference = themePreference => { | ||
| const resolvedTheme = | ||
| themePreference === 'system' ? getSystemTheme() : themePreference; | ||
|
|
||
| document.documentElement.setAttribute('data-theme', resolvedTheme); | ||
| document.documentElement.style.colorScheme = resolvedTheme; | ||
| }; | ||
|
|
||
| /** | ||
| * A React hook for managing the application's theme preference. | ||
| */ | ||
| export const useTheme = () => { | ||
| const [theme, setTheme] = useState('light'); | ||
| const [themePreference, setThemePreferenceState] = useState('system'); | ||
|
|
||
| useEffect(() => { | ||
| const initial = | ||
| // Try to get the theme from localStorage first. | ||
| localStorage.getItem('theme') || | ||
| // If not found, check the `data-theme` attribute on the document element | ||
| document.documentElement.getAttribute('data-theme') || | ||
| // As a final fallback, check the user's system preference for dark mode. | ||
| (matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'); | ||
|
|
||
| applyTheme(initial); | ||
| setTheme(initial); | ||
| // Use persisted preference if available, otherwise default to system. | ||
| const initialPreference = getStoredThemePreference() || 'system'; | ||
|
|
||
| applyThemePreference(initialPreference); | ||
| setThemePreferenceState(initialPreference); | ||
| }, []); | ||
|
|
||
| /** | ||
| * Callback function to toggle between 'light' and 'dark' themes. | ||
| * Keep the resolved document theme in sync with system changes | ||
| * whenever the preference is set to 'system'. | ||
| */ | ||
| useEffect(() => { | ||
| if (themePreference !== 'system') { | ||
| return; | ||
| } | ||
|
|
||
| const mediaQueryList = matchMedia('(prefers-color-scheme: dark)'); | ||
| /** | ||
| * | ||
| */ | ||
| const handleSystemThemeChange = () => applyThemePreference('system'); | ||
|
|
||
| if ('addEventListener' in mediaQueryList) { | ||
| mediaQueryList.addEventListener('change', handleSystemThemeChange); | ||
| return () => { | ||
| mediaQueryList.removeEventListener('change', handleSystemThemeChange); | ||
| }; | ||
| } | ||
|
|
||
| mediaQueryList.addListener(handleSystemThemeChange); | ||
| return () => { | ||
| mediaQueryList.removeListener(handleSystemThemeChange); | ||
| }; | ||
|
Comment on lines
+85
to
+95
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addEventListener is standard, is it not? We don't need a backup check
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList so we can remove the back-up change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be careful about Safari support here, esp. on iOS: https://caniuse.com/mdn-api_mediaquerylist_change_event. Support only landed in late 2020. |
||
| }, [themePreference]); | ||
|
|
||
| /** | ||
| * Updates the theme preference and applies it immediately. | ||
| */ | ||
| const toggleTheme = useCallback(() => { | ||
| setTheme(prev => { | ||
| // Determine the next theme based on the current theme. | ||
| const next = prev === 'light' ? 'dark' : 'light'; | ||
| // Apply the new theme. | ||
| applyTheme(next); | ||
| // Return the new theme to update the state. | ||
| return next; | ||
| }); | ||
| const setThemePreference = useCallback(nextPreference => { | ||
| if (!THEME_PREFERENCES.has(nextPreference)) { | ||
| return; | ||
| } | ||
|
Comment on lines
+102
to
+104
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this ever happen?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no we can remove it |
||
|
|
||
| setThemePreferenceState(nextPreference); | ||
| setStoredThemePreference(nextPreference); | ||
| applyThemePreference(nextPreference); | ||
| }, []); | ||
|
|
||
| return [theme, toggleTheme]; | ||
| return [themePreference, setThemePreference]; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just assume that the theme preference is correct, no need for the ternary or catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are proposing to just return localStorage.getItem ?