-
Notifications
You must be signed in to change notification settings - Fork 11
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
Remove react hook and use javascript to detect theme state #15
base: master
Are you sure you want to change the base?
Conversation
|
||
const IndexPage = () => { | ||
|
||
configureDarkMode(); |
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 just thinking not tried but is it working when we go to another page? It looks like it is working on index page but what happens if we go to download page? Is it still works because it looks like you are just adding dark
class on index page
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.
Good catch, I didn't even think about that. It seems to maintain state when navigating to the download page, which makes sense, but then upon reload defaults to light. I added the method to all pages and now it works fine, no flash and respects system theme, for all of them (even upon changing in system settings and navigating to the page.)
src/libs/configureDarkMode.js
Outdated
import "react"; | ||
|
||
const colorSchemeMedia = "(prefers-color-scheme: dark)"; | ||
const isBrowser = () => typeof window !== "undefined" |
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.
Extract this into some utility file and also use it in CustomLink
/Button
.
const isBrowser = () => typeof window !== "undefined" | ||
|
||
const configureDarkMode = () => { | ||
|
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.
Extra space.
src/libs/configureDarkMode.js
Outdated
|
||
// TODO: Add a switch that respects local storage and allows user to choose |
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.
If something is fully functional in itself then there's no need to add TODOs.
|
||
const IndexPage = () => { | ||
|
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.
Extra space.
const Layout = ({ children, title }) => { | ||
const isDark = useDarkModeState(); | ||
|
||
const Layout = ({ children, title}) => { | ||
|
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.
Extra space.
This fixes the initial flash that can be seen when opening the page and using a system default of dark mode.
In the future, I'd like to add local storage based dark mode handling, so this opens that up as well.