-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: resolve dark mode flicker issue #7533
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: master
Are you sure you want to change the base?
Changes from 8 commits
a5c4a7d
8d09e77
c78a6f9
7642946
21df90e
ed0aa74
9ad62cb
f60787a
4491cc1
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 |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ const isBrowser = typeof window !== "undefined"; | |
|
|
||
| const systemDarkModeSetting = () => | ||
| isBrowser && window.matchMedia ? window.matchMedia("(prefers-color-scheme: dark)") : null; | ||
|
|
||
| const isDarkModeActive = () => { | ||
| return !!systemDarkModeSetting()?.matches; | ||
| }; | ||
|
|
@@ -36,29 +37,37 @@ const applyThemeToDOM = (theme) => { | |
| const root = window.document.documentElement; | ||
| root.style.setProperty("--initial-color-mode", theme); | ||
| root.setAttribute("data-theme", theme); | ||
| window.__theme = theme; | ||
| }; | ||
|
|
||
| export const ThemeManagerProvider = (props) => { | ||
| const [themeSetting, setThemeSetting] = useState(ThemeSetting.SYSTEM); | ||
| const [didLoad, setDidLoad] = useState(false); | ||
| const [isDark, setIsDark] = useState(false); | ||
|
|
||
| const [isDark, setIsDark] = useState(() => { | ||
| if (isBrowser) { | ||
| if (window.__theme === ThemeSetting.DARK) return true; | ||
| if (window.__theme === ThemeSetting.LIGHT) return false; | ||
| } | ||
| return false; | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!isBrowser) return; | ||
|
|
||
| const root = window.document.documentElement; | ||
| const initialColorValue = root.style.getPropertyValue("--initial-color-mode"); | ||
| const initialColorValue = (root.style.getPropertyValue("--initial-color-mode") || "").trim(); | ||
| const actualTheme = window.__theme || initialColorValue || ThemeSetting.LIGHT; | ||
AnkitRewar11 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Get stored theme from localStorage | ||
|
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. Please don’t remove the comments.
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. Hi @rishiraj38, I hope everything got added properly. I’ve added that one comment as you suggested. |
||
| const storedTheme = localStorage.getItem(DarkThemeKey); | ||
|
|
||
| if (storedTheme && storedTheme !== ThemeSetting.SYSTEM) { | ||
| const isDarkTheme = storedTheme === ThemeSetting.DARK; | ||
| setIsDark(isDarkTheme); | ||
| setThemeSetting(storedTheme); | ||
| applyThemeToDOM(storedTheme); | ||
| } else if (initialColorValue) { | ||
| setIsDark(initialColorValue === ThemeSetting.DARK); | ||
| } else if (actualTheme) { | ||
| setIsDark(actualTheme === ThemeSetting.DARK); | ||
| setThemeSetting(ThemeSetting.SYSTEM); | ||
| } else { | ||
| // Fallback to system preference | ||
|
|
@@ -71,7 +80,7 @@ export const ThemeManagerProvider = (props) => { | |
| setDidLoad(true); | ||
| }, []); | ||
|
|
||
| // Listen to system color scheme changes only when on SYSTEM mode | ||
| // Listen to system color scheme changes only when on SYSTEM mode | ||
| useEffect(() => { | ||
| if (!isBrowser || themeSetting !== ThemeSetting.SYSTEM) return; | ||
|
|
||
|
|
@@ -93,11 +102,11 @@ export const ThemeManagerProvider = (props) => { | |
| const newIsDark = !isDark; | ||
| const newTheme = newIsDark ? ThemeSetting.DARK : ThemeSetting.LIGHT; | ||
|
|
||
| // Update state | ||
| // Update state | ||
| setIsDark(newIsDark); | ||
| setThemeSetting(newTheme); | ||
|
|
||
| // Apply to DOM immediately | ||
| // Apply to DOM immediately | ||
| applyThemeToDOM(newTheme); | ||
|
|
||
| // Persist to localStorage | ||
|
|
@@ -129,14 +138,14 @@ export const ThemeManagerProvider = (props) => { | |
| return; | ||
| } | ||
|
|
||
| // Update state | ||
| // Update state | ||
| setIsDark(newIsDark); | ||
| setThemeSetting(setting); | ||
|
|
||
| // Apply to DOM immediately | ||
| applyThemeToDOM(themeToApply); | ||
|
|
||
| // Persist to localStorage | ||
|
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. Reinstate the comments. |
||
| // Persist to localStorage | ||
| localStorage.setItem(DarkThemeKey, setting); | ||
| }, | ||
| [isDark] | ||
|
|
||
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.
Why was the existing approach completely ripped out and replaced?
What is precisely wrong with the current approach? Fix that.
If the current approach is completely offbase - ok - explain that and why a complete rewrite is needed.
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.
@AnkitRewar11 can you answer this?
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.
Hey @saurabhraghuvanshii, just wanted to clearly explain why the master branch approach was failing and how this PR fixes it.
I’ve only made a few focused fixes to solve the dark mode flicker issue:
Wrong Injection Placement: The script was running too late (inside ), so the page first showed light mode. I moved it to so the correct theme loads before anything is displayed.
Missing data-theme: The code wasn’t setting the data-theme attribute on the HTML root, which is needed for dark mode styles. I’ve added that.
SSR / Hydration Mismatch: The server knew the theme, but the frontend didn’t, so it defaulted to light mode and caused flicker. I fixed this by passing the theme (window.__theme) so both stay in sync.
JSON Parsing Issue: The previous way of parsing theme data could break if there were quotes in the string. I made it safer to avoid crashes.
Now the changes are minimal, clean, and focused and the flicker issue is completely resolved. I hope all your questions have been answered. If there’s anything else you’d like to know, please let me know.