-
Notifications
You must be signed in to change notification settings - Fork 162
Add dark mode theme to the UI #3677
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
Conversation
I know it's a draft but the tests are failing with message:
You might want to look into it earlier. |
Does it mean they are positioned incorrectly? |
The icon's BG is not transparent? Wow. |
Ah, maybe this AlertProps error is related to the following message you removed:
I wonder why it was added before. |
Haha, I like the light/dark mode switch! ✨ |
Tested it locally, the dark mode looks very cool in general! 🎉 Great job! |
Important notes written up by @LappleApple: #2948 (comment) |
Updated toastify to get access to their theme prop: Screen.Recording.2023-05-10.at.10.52.12.AM.mov |
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.
UI Looks fantastic 👍 .
I saw some repeated logic and I have a couple questions.
@@ -374,7 +374,10 @@ export const DataTable = styled(UnstyledDataTable)` | |||
transition: background 0.5s ease-in-out; | |||
} | |||
.MuiTableRow-root:not(.MuiTableRow-head):hover { | |||
background: ${(props) => props.theme.colors.neutral10}; | |||
background: ${(props) => | |||
props.theme.colors.black === "#fff" |
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 assume this is checking to see if we are in dark mode. Can we instead do props.theme.name === 'dark'
or something like that?
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.
Added a new field to the DefaultTheme
type called mode
that uses the ThemeTypes
enum
ui/components/Logo.tsx
Outdated
@@ -13,14 +14,16 @@ type Props = { | |||
}; | |||
|
|||
function Logo({ className, link, collapsed }: Props) { | |||
const { settings } = React.useContext(AppContext); | |||
const dark = settings.theme === ThemeTypes.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.
We are repeating this in multiple places. Can we put this in a reusable hook?:
const dark = settings.theme === ThemeTypes.Dark; | |
const theme = useGetThemeSetting() | |
if (theme === ThemeTypes.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.
Added useInDarkMode
hook which returns a boolean
@@ -10,15 +10,22 @@ type AppState = { | |||
detailModal: DetailViewProps; | |||
}; | |||
|
|||
export enum ThemeTypes { |
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.
👍
ui/images/signInWheelDark.svg
Outdated
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.
🔥 🔥 🔥
ui/components/ChipGroup.tsx
Outdated
{chips.length > 0 && ( | ||
<Chip | ||
label="Clear All" | ||
className="filter-options-chip" |
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.
💡 It would be cleaner to have filter-options-chip
class in ChipGroup
too if possible. It is not supposed to know about a class name in DataTable.
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 class and just moved the bg color into the component itself...not sure why it was added originally. Looked in enterprise too for a reason and I don't see one. Should be okay.
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.
Tested it locally, works and looks great! 🎉
…ill a little polishing left to do
… toastify, signin eye icon
…lter dialog button, and detail modal background
…overrides for disabled checkbox and input
…treamline colors a bit
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.
LGTM 👍
<Link to={link || V2Routes.Automations}> | ||
<img src={images.logotype} /> | ||
<Link to={link}> | ||
<img src={dark ? images.logotypeLight : images.logotype} /> |
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.
💯
Closes #3670
Screen.Recording.2023-05-10.at.3.51.16.PM.mov
Toastify update:


Colors guide:
black
andwhite
switch as you'd expect.neutral00
-neutral40
reverse order, which meansneutral20
stays the same.primary
switches withprimary10
NOTprimary20
. Primary20 was not being used, but I used it for an override on dark mode disabled buttons. It's used in one tiny lil thing in Enterprise which I'll address if I need to.primaryLight05
was only being used to hide text in the Nav when collapsed - in dark mode it switches to a color that does the same. I believe it's being used for something in Enterprise and needs to be addressed.primaryLight10
was not being used in OSS so I've left it as is for now.alertLight
is what was being used in OSS for all error stuff, it switches toalertDark
for dark mode.backGray
, which was being used for the top bar, now switches to theCorporateCharcoal
purple-ish color used all over dark mode. (#32324B
)whiteToPrimary
is for when you need white to switch to theCorporateCharcoal
grayToPrimary
changesneutral30
toprimary10
blueWithOpacity
is a special override to emulate the hover effect on Nav items in the dark mode DataTable