-
Notifications
You must be signed in to change notification settings - Fork 228
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
feat: add dark theme support #430
Conversation
@ecmchow this is really cool but it's a big change, so I will need to take some time to go through it Thanks for the work on this |
hey @ecmchow just a heads up that I have asked one of our designers to look at your PR to make sure they are happy they might ask for some color changes but we will address that if/when it happens :) |
@d1wilko Thanks for letting me know. I've kind of guessed the purpose of these color and hand-picked the dark theme color by reducing lightness only, so it might not fit the design system/standard. So feel free to adjust the color and variable names to your needs :) |
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 @ecmchow great work on this - some suggestions to improve it a little :)
src/styles/_variables.scss
Outdated
// Gray | ||
$color-gray-200: #d9dae5; | ||
$color-gray-300: #b0b1c3; | ||
$color-gray-400: #9293ab; | ||
$color-gray-400: #9293ab;; |
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.
does this need 2 semi ciolons?
@@ -1,11 +1,14 @@ | |||
import classNames from 'classnames/bind'; | |||
import PropTypes from 'prop-types'; | |||
import React, { useState } from 'react'; | |||
import React, { useEffect, useState } from 'react'; |
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.
is useEffect needed here?
src/hooks/use-theme.js
Outdated
import { useEffect, useCallback, useState } from 'react'; | ||
|
||
import useLocalStorage from './use-local-storage'; | ||
|
||
const useTheme = () => { | ||
const [storedTheme, setStoredTheme] = useLocalStorage('selected_theme', 'system'); | ||
const [selectedTheme, setSelectedTheme] = useState(storedTheme); | ||
|
||
const isSystemThemeDark = () => window.matchMedia('(prefers-color-scheme: dark)').matches === true; | ||
|
||
useEffect(() => { | ||
if ( | ||
(selectedTheme === 'system' && isSystemThemeDark()) || (selectedTheme === 'dark') | ||
) { | ||
document.body.classList.add('dark'); | ||
} else { | ||
document.body.classList.remove('dark'); | ||
} | ||
}, [selectedTheme]); | ||
|
||
const toggleTheme = useCallback(() => { | ||
let newTheme = 'system'; | ||
if (selectedTheme === 'system') { | ||
newTheme = isSystemThemeDark() ? 'light' : 'dark'; | ||
} else { | ||
newTheme = selectedTheme === 'dark' ? 'light' : 'dark'; | ||
} | ||
|
||
setStoredTheme(newTheme); | ||
setSelectedTheme(newTheme); | ||
}, [selectedTheme, setStoredTheme, setSelectedTheme]); | ||
|
||
return { selectedTheme, toggleTheme }; | ||
}; | ||
|
||
export default 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.
I think we can simplify this a bit - there no need for state AND localStorage - localStorage should suffice
something like this could work
import { useEffect } from 'react';
import useLocalStorage from './use-local-storage';
const DARK = 'dark';
const LIGHT = 'light';
const useTheme = () => {
const isSystemThemeDark = () => window.matchMedia('(prefers-color-scheme: dark)').matches === true;
const [storedTheme, setStoredTheme] = useLocalStorage('selected_theme', isSystemThemeDark ? DARK : LIGHT);
useEffect(() => {
if (
(storedTheme === DARK)
) {
document.body.classList.add(DARK);
} else {
document.body.classList.remove(DARK);
}
}, [storedTheme]);
const toggleTheme = () => {
setStoredTheme(storedTheme === DARK ? LIGHT : DARK);
};
return { storedTheme, toggleTheme };
};
export default useTheme;
@@ -58,6 +62,18 @@ export default function Sidebar(props) { | |||
|
|||
const bottomPart = ( | |||
<div className={cx('bottom')}> | |||
<Button | |||
className={cx('sidebar-item', 'theme-btn')} | |||
theme="plain" |
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 not sure what the theme
is?
label="Search" | ||
onClick={toggleTheme} | ||
> | ||
{selectedTheme === 'dark' ? ( |
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 you change the hook - this will need to change :)
@d1wilko Thanks for the suggestions. I've simplified the theme hook and cleanup the style a bit, please have a look and let me know if there is more comments :) |
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.
looks great thank you
I need to make some changes to the colors as per our UX guys but I will do that and add you as a reviewer before we do a proper release
Added dark theme support
prefers-color-scheme
as default valueScreenshots: