fix: Moved language toggle to themeContext and update theme.direction on language change #991
Conversation
…e theme when darkMode or direction changes
…ce actual toggle logic moved to context
…since it is now globally updated
TomRytt
changed the title
src/layout/ThemeContext.tsx
Outdated
| const { defaultAlgorithm, darkAlgorithm } = theme | ||
|
|
||
| export const ThemeProvider: FC<PropsWithChildren> = ({ children }) => { | ||
| const [isDarkTheme, setIsDarkTheme] = useLocalStorage<boolean>('isDarkTheme') | ||
|
|
||
| const [direction, setDirection] = useState<'ltr' | 'rtl'>('rtl') |
There was a problem hiding this comment.
do we need a state variable here? sounds like we can use derived state here
There was a problem hiding this comment.
When I checked previously with a useEffect on the i18n.language to see where it is changing and re-create the theme.direction based on it, it didn't catch those changes so I created this state to update when changing language. I'll test it again and remove it if i can successfully derive the direction from the state.
There was a problem hiding this comment.
I think it can be calculated without hooks at all - it's a primitive string so there's no harm in recalculating it without memoization (unlike object references that could trigger unnecessary renders )
There was a problem hiding this comment.
You were right. I fixed this in both branches by mistake. Is that still ok to merge them now?
There was a problem hiding this comment.
@NoamGaash Are the new changes better? Can I merge now?
There was a problem hiding this comment.
@TomRytt sure, thanks! once everything is approved and passing merging is great :)
Description
*Not sure if this should be filed under refactor or fix.
Moved logic from LanguageToggle to themeContext for four main advantages:
screenshots