-
Notifications
You must be signed in to change notification settings - Fork 421
RI-7754 detect system color change #5268
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
Conversation
… in the rare case, that it didn't got initialized on module load
Code Coverage - Frontend unit tests
Test suite run success5474 tests passing in 705 suites. Report generated by 🧪jest coverage report action from 0c9a64c |
| export const useSystemThemeListener = () => { | ||
| const { usingSystemTheme, changeTheme } = useThemeContext() | ||
|
|
||
| const handleSystemThemeChange = useCallback(() => { | ||
| usingSystemTheme && changeTheme(Theme.System) | ||
| }, [changeTheme, usingSystemTheme]) | ||
|
|
||
| useEffect(() => { | ||
| if (!mediaQuery) { | ||
| // Initialize mediaQuery if not done already because window might not be defined when module is loaded | ||
| mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK) | ||
| } | ||
| // Only listen if using system theme | ||
| if (usingSystemTheme) { | ||
| if (!mediaQuery) { | ||
| return undefined | ||
| } | ||
| mediaQuery.addEventListener('change', handleSystemThemeChange) | ||
| } | ||
| return () => { | ||
| if (mediaQuery) { | ||
| mediaQuery.removeEventListener('change', handleSystemThemeChange) | ||
| } | ||
| } | ||
| }, [usingSystemTheme]) | ||
| } |
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'm noticing some issues with this implementation.
The goal is to attach an event listener in case the theme is set to SYSTEM that would monitor the respective media match and change theme acordingly.
- handleSystemThemeChange is memoized callback, however its two dependencies are changeTheme (stable, does not change), usingSystemTheme (same as the useEffect one's). Also it is not listed as dependency of the useEffect - not clear that it's supposed to be redefined first or the effect is first, plus it's used as cleanup - hard to follow lifecycle. Finally, since It is not used outside the useEffect and has the same dependencies, it is preferable that it's defined inside the useEffect as well.
- The useEffect has the same check as the one in handleSystemThemeChange (usingSystemTheme) - it only adds the event listener if usingSystemTheme is true, meaning the check inside handleSystemThemeChange is redundant
- The useEffect returns a cleanup function regardless of weather or not an event was attached
I'm suggesting a much simplified solution that should perform the same task and addresses the point mentioned above:
| export const useSystemThemeListener = () => { | |
| const { usingSystemTheme, changeTheme } = useThemeContext() | |
| const handleSystemThemeChange = useCallback(() => { | |
| usingSystemTheme && changeTheme(Theme.System) | |
| }, [changeTheme, usingSystemTheme]) | |
| useEffect(() => { | |
| if (!mediaQuery) { | |
| // Initialize mediaQuery if not done already because window might not be defined when module is loaded | |
| mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK) | |
| } | |
| // Only listen if using system theme | |
| if (usingSystemTheme) { | |
| if (!mediaQuery) { | |
| return undefined | |
| } | |
| mediaQuery.addEventListener('change', handleSystemThemeChange) | |
| } | |
| return () => { | |
| if (mediaQuery) { | |
| mediaQuery.removeEventListener('change', handleSystemThemeChange) | |
| } | |
| } | |
| }, [usingSystemTheme]) | |
| } | |
| export const useSystemThemeListener = () => { | |
| const { usingSystemTheme, changeTheme } = useThemeContext() | |
| useEffect(() => { | |
| const mediaQuery = window.matchMedia?.(THEME_MATCH_MEDIA_DARK) | |
| // Only listen if using system theme | |
| if (usingSystemTheme && mediaQuery) { | |
| const handleSystemThemeChange = () => changeTheme(Theme.System) | |
| mediaQuery.addEventListener('change', handleSystemThemeChange) | |
| return () => { | |
| mediaQuery.removeEventListener('change', handleSystemThemeChange) | |
| } | |
| } | |
| return undefined | |
| }, [usingSystemTheme, changeTheme]) | |
| } |
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.
This will trigger non-consistent return points warning in tslint, that is why I prefer the more verbose and defensive style I used.
Better safe than sorry
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.
ok, add return undefined in the end like you did, eslint is the not related to my concerns above
(edit: updated suggestion above)
(edit2: in js every void function returns undefined)
Remove module-level variable and useCallback wrapper. Simplify event listener setup and cleanup logic. References: #RI-7754
Remove module-level variable and useCallback wrapper. Simplify event listener setup and cleanup logic. References: #RI-7754
What
Add automatic system theme change detection. When user selects "System" theme, the app now automatically updates when the OS theme changes from dark to light (or vice versa).
useSystemThemeListenerhook that listens toprefers-color-schememedia query changesApp.tsxto monitor system theme changesTesting
Closes #RI-7754
Note
Automatically updates app theme when the OS switches light/dark by adding a system theme listener hook and wiring it into App.
useSystemThemeListenerhook to listen toprefers-color-schemechanges and callchangeTheme(Theme.System).ui/src/App.tsxto react to system theme changes when using System theme.Written by Cursor Bugbot for commit 0c9a64c. This will update automatically on new commits. Configure here.