Dark mode #21
Dark mode #21
Conversation
- New icons Sun + Moon
- utils/colorScheme.ts
- Detects browser settings
- Overrides browser settings on click
- index.css
- Implemented dark mode
| import { detectColorScheme, switchTheme } from '../util/colorScheme'; | ||
|
|
||
| const ColorSchemeToggle = () => { | ||
| const [dark, setDark] = useState<boolean>(); |
There was a problem hiding this comment.
Should be explicitly "light", "dark" or "system"
There was a problem hiding this comment.
This value should also not be managed by a component, instead I suggest using context
| }, []); | ||
|
|
||
| const handleClick = (): void => { | ||
| setDark((_prev) => !_prev); |
There was a problem hiding this comment.
Leading with an underscore in variable name is generally used to mark a variable as unused (this is the way VSC deals with it). Also the more explicit previous would be preferred.
| @@ -8,6 +8,8 @@ import { useSearch } from '../context/searchContext'; | |||
| import FilterModal from './FilterModal'; | |||
| import Search from '../icons/Search'; | |||
| import Clear from '../icons/Clear'; | |||
| import { detectColorScheme, switchTheme } from '../util/colorScheme'; | |||
| --primary: 64, 104, 125; | ||
| --primary-medium: 46, 81, 99; | ||
| --primary-dark: 28, 42, 51; |
There was a problem hiding this comment.
Should also be altered using dark mode to ensure good contrast
| overflow-x: hidden; | ||
| text-overflow: ellipsis; | ||
| > li { | ||
| background-color: rgba(var(--background)); |
| import React from 'react'; | ||
|
|
||
| function detectColorScheme(): boolean { | ||
| var theme = 'light'; //default to light |
There was a problem hiding this comment.
Theme should be typed to 'light' | 'dark'.
| if (localStorage.getItem('theme')) { | ||
| if (localStorage.getItem('theme') == 'dark') { |
There was a problem hiding this comment.
Would prefer if the LS key was defined as a const and specified to this application (as seen in bookmarksContext to remove risk of collision when on localhost). E.g. const LS_THEME_KEY = 'in-songbook:theme'
| } else if (!window.matchMedia) { | ||
| //matchMedia method not supported | ||
| return false; | ||
| } else if (window.matchMedia('(prefers-color-scheme: dark)').matches) { |
There was a problem hiding this comment.
This could be one case window.matchMedia?.('(prefers-color-scheme: dark)')
| if (theme == 'dark') { | ||
| document.documentElement.setAttribute('data-theme', 'dark'); | ||
| } |
There was a problem hiding this comment.
Since this is only to detect what color scheme to use, this function should not set data-theme
| return theme === 'dark'; | ||
| } | ||
|
|
||
| function switchTheme(dark: boolean) { |
There was a problem hiding this comment.
Should be using explicit strings instead of a boolean.
No description provided.