-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Use dark
class for color mode selector
#13661
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
export const useNav = () => { | ||
const { t } = useTranslation("common") | ||
const { theme, setTheme } = useTheme() | ||
const { setTheme, resolvedTheme, systemTheme } = useTheme() |
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.
Removed usage of theme
which was not always responding appropriately to system color preference changes.
In it's place, resolvedTheme
is a more reliable "light" or "dark" string that the theme will resolve to. Also, systemTheme
taps into the color mode preference of the system, and updates on change.
const lsTheme = targetTheme === systemTheme ? "system" : targetTheme | ||
|
||
setTheme(lsTheme) |
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.
When observing this in the Application dev tools, the "system" is automatically parsed (by the embedded next-theme
I believe*) to "light" or "dark". Using this approach seems to make sure the html[class]
is updated by the NextThemeProvider.
*embedded `next-theme` script
!(function () {
try {
var d = document.documentElement,
c = d.classList;
c.remove('light', 'dark');
var e = localStorage.getItem('theme');
if ('system' === e || (!e && true)) {
var t = '(prefers-color-scheme: dark)',
m = window.matchMedia(t);
if (m.media !== t || m.matches) {
d.style.colorScheme = 'dark';
c.add('dark');
} else {
d.style.colorScheme = 'light';
c.add('light');
}
} else if (e) {
c.add(e || '');
}
if (e === 'light' || e === 'dark') d.style.colorScheme = e;
} catch (e) {}
})();
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.
@wackerow a few things related to this PR and the color theme mechanism
-
I pushed a new PR Fix color themes sync #13676 to sync the color themes between chakra and next-themes. Maybe this PR will clarify a bit what is going on between the two color theme syncs.
-
Left a comment/question/opinion here about the manual system color selection.
-
On choosing the
class
method, would you explain again why we would prefer this over thedata-
attr? question because chakra will automatically detect the latter but not the class and that might save us from manually sync the color themes.
=====
Saw this issue in the preview deploy
I guess this issue is related to the usage of theme
instead of resolvedTheme
(fixed in #13663) as this branch is outdated.
useEffect(() => { | ||
setTheme("system") | ||
setColorMode(systemTheme) | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [systemTheme]) |
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 understand the intention here to be aligned with the behavior we had before with Chakra but we are getting 2 issues from this:
- An extra render
- Forcing the user to system color every time the page is loaded which IMO is not a good UX
IMO we should consider adding a new option in our toggle button to consider the system
preference (as this is how next-themes
was designed https://next-themes-example.vercel.app/). For some users, it might be weird that the color mode changes automatically if they manually choose a specific color mode.
Closing out; avoiding |
Description
.dark
class for color mode managementPreview URL
Related Issue
Tailwind migration