feat: Dark Mode #718

lanesawyer · 2023-11-29T19:21:52Z

Dark Mode

Structural Element (Base, Gene, DNA, Chromosome or Cell)
Github issue: #XXXX
Copy isuue descirption here

Remaining Work

Real dark mode colors
Checking all the components to make sure nothing is broken
Accessibility verification
Updated documentation for setting up the project since you'll need to provide a theme now (or, we just default to light or something)
Probably other things I'm forgetting

Checklist

Default Story in Storybook
LivePreview Story in Storybook
Test Story in Storybook
Tests written
Variables from defaultTheme.ts used wherever possible
If updating an existing component, depreciate flag has been used where necessary
Chromatic build verified by @chanzuckerberg/sds-design

lanesawyer · 2023-11-29T19:26:08Z

.gitignore

@@ -108,3 +108,12 @@ storybook-static

 # OS Files
 .DS_Store
+


Turns out I'm on the latest version of yarn which hasn't been used by this project yet so I was getting tons of added files from yarn's cache. This ensures the latest yarn doesn't cause git annoyances in the future 😄

packages/components/src/core/ButtonIcon/index.stories.tsx

lanesawyer · 2023-11-29T19:27:54Z

packages/components/src/core/Callout/style.ts

@@ -39,12 +38,13 @@ export const StyledCallout = styled(Alert, {
    // any buttom margin
    const titleBottomMargin = props.collapsed ? "margin-bottom: 0;" : "";

+    // TODO: Theme shouldn't be saying it might be null


Type issue, needs to be addressed.

So this TODO is a little right and a little wrong, looking at it again.

The props here are listed as CalloutProps, which extends from the CommonThemeProps where theme is undefined.

At runtime, theme would never be undefined, so that's why I dropped the initial TODO here.

I'm sure we could do a bit of type juggling we could do to make it defined, but I think we'd want to do that in other components that are extending CommonThemeProps directly.

Given this PR is more targeted at making dark mode work, I'm inclined to live with the untrue ? on theme for now.

packages/components/src/core/LoadingIndicator/index.stories.tsx

lanesawyer · 2023-11-29T19:28:34Z

packages/components/src/core/styles/common/defaultTheme.ts

@@ -75,8 +75,64 @@ const defaultThemeColors = {
  },
 };

-export const defaultAppTheme: AppTheme = {
-  colors: defaultThemeColors,
+const darkThemeColors = {


Gross ChatGPT generated ones, will be replaced.

packages/components/src/core/styles/common/defaultTheme.ts

lanesawyer · 2023-11-29T21:43:09Z

README.md

@@ -94,7 +94,7 @@ export const Title - styled(Typography)`
 ```ts
 import { css, SerializedStyles } from "@emotion/react";
 import { styled } from '@mui/material/styles';
-import { getColors, getCorners } from "@czi-sds/components";
+import { getColors, getSpaces } from "@czi-sds/components";


Just fixing a typo in the README

lanesawyer · 2023-11-29T22:34:37Z

packages/components/src/core/styles/common/defaultTheme.ts

+/**
+ * Default theme, uses light mode with no option to change it.
+ *
+ * @deprecated Use the `theme` to get a flexible light/dark mode theme function


Maybe we don't deprecate it, some folks may never be interested in light/dark so why complicate their lives?

Alternatively, deprecate but give the new one a default theme of "light". 🤔

Great question! Yeah I also feel that maybe don't deprecate it and just default to "light" sounds less intrusive 😄 👍

- Updated DialogTitle to use palette.text colors - Updated Tabs to use palette.text colors

lanesawyer · 2023-11-29T23:19:41Z

packages/components/src/core/styles/common/defaultTheme.ts

+  900?: string;
+  800?: string;
+  700?: string;


If we don't want all Color types to be able to climb this high we could make an ExtendedColor interface that extends Color and adds these three. Then it can be applied only where these values would be desired.

lanesawyer · 2023-11-30T00:49:23Z

packages/components/src/core/DialogTitle/style.ts

-  color: black;
+  ${(props) => {
+    return `
+      color: ${props.theme.palette.text.primary};


Just thinking about this a bit... Typography should be theme-aware for the text.primary color. I think we might be able to get away with removing color entirely!

Thanks for paving the way for our components to be runtime theme aware 💡 !!

Yeah using semantic text.primary sounds like best practice to me too 👍 !

Cool, going to remove the color entirely for Title then. Keeping for Subtitle since that won't be a secondary color by default!

tihuan

Looks SUPER good! Thanks so much for going through the code to find places where we can start adopting runtime theme swapping!

These examples will definitely help us overhaul the rest of the stories and components in Q2 👏

Really appreciate it!

Just some minor questions/feedbacks, otherwise LGTM 🚀 !!

tihuan · 2024-01-19T21:44:53Z

packages/components/src/core/Tabs/style.tsx

@@ -51,7 +51,9 @@ export const StyledTabs = styled(TempTabs, {
    return `
      margin-top: ${isLarge ? spaces?.l : spaces?.m}px;
      margin-bottom: ${isLarge ? spaces?.xl : spaces?.m}px;
-      border-bottom: ${underlined ? `2px solid ${colors?.gray[200]};` : "none"};
+      border-bottom: ${
+        underlined ? `2px solid ${props.theme.palette.divider};` : "none"


super nice!

tihuan · 2024-01-19T21:46:45Z

packages/components/src/core/styles/common/defaultTheme.ts

+  colors: lightThemeColors,
+  ...sharedAppTheme,


should we swap the order here, so we ensure colors: lightThemeColors, will overwrite sharedAppTheme.colors if sharedAppTheme happens to have colors at runtime 😄 ?

Good point! Even though I've said sharedAppTheme omits colors, that doesn't mean it'll actually happen.

The joys of TypeScript lol

Swapping!

tihuan · 2024-01-19T21:49:40Z

packages/components/src/core/styles/common/defaultTheme.ts

+/**
+ * Default theme, uses light mode with no option to change it.
+ *
+ * @deprecated Use the `theme` to get a flexible light/dark mode theme function


Great question! Yeah I also feel that maybe don't deprecate it and just default to "light" sounds less intrusive 😄 👍

tihuan · 2024-01-19T21:51:36Z

packages/components/src/core/styles/common/defaultTheme.ts

+ * @deprecated Use the `theme` to get a flexible light/dark mode theme function
+ */
+export const defaultTheme = createThemeAdaptor(
+  makeThemeOptions(chooseTheme("light"), "light")


feels like potentially defaultTheme() could call theme() under the hood?

Something like:

export const defaultTheme = theme("light");

What do you think 🙆‍♂️ ?

lanesawyer · 2024-01-19T22:36:33Z

packages/components/src/core/styles/common/defaultTheme.ts

+  divider: appTheme.colors.gray[200],
+  background: {
+    default: common.white,
+    secondary: appTheme.colors.gray[100],


I think this was added by @hthomas-czi during the hackathon, it's throwing a type error because we don't have it on TypeBackground.

@hthomas-czi Is a secondary background still something that's needed? There's paper that we could be using that already exists, so we'll have three background colors if we do use secondary.

If it's needed, it's easy to add to the type. I just want to check 😁

I think this was added by @hthomas-czi during the hackathon, it's throwing a type error because we don't have it on TypeBackground.

@hthomas-czi Is a secondary background still something that's needed? There's paper that we could be using that already exists, so we'll have three background colors if we do use secondary.

If it's needed, it's easy to add to the type. I just want to check 😁

Hey @lanesawyer! Sorry for the delay responding. For some horrible reason, I don't receive notifications for GitHub mentions.

I'll leave it up to you, but Paper doesn't fully address the reason I added a secondary background. They're equivalent in dark mode. But in light mode, Paper is always white, while background[secondary] is light gray in light mode. This maintains a consistent appearance between modes.

Thanks!

lanesawyer · 2024-01-19T22:40:56Z

@tihuan Feedback addressed! I have an outstanding question for @hthomas-czi, but overall this PR should be pretty close to ready.

Also one more question, should we add details about the theme to the README in this PR, or update the README once the colors and polish are completed so people won't start thinking it's ready?

Finally, I guess my PR doesn't trigger the workflow. Probably want to approve those to run so we can check that I didn't miss anything. Running the linter locally, I got a bunch of errors saying Cannot find module @czi-sds/components' or its corresponding type declarations., but everything in Storybook seemed to be working. Not sure if that's my machine's problem, if I'm running the linter incorrectly (I'm doing yarn lint), or what.

tihuan

Awesome! Thanks for addressing the feedback quickly, Lane 🫡 LGTM!

CC: @masoudmanson in case I missed something 😎🙏

Thanks again!

masoudmanson

Wow, @lanesawyer, this is truly fantastic!
A massive thank you for tackling the heavy lifting and crafting such a robust PR for us. Really appreciate it.
The PR looks great to me. I'll be continuing to update components and collaborate with the design team to fine-tune the colors and appearance of the components.
This is so exciting! 🥳

Finally, I feel right at home as a programmer. We got a dark theme 😱😍

masoudmanson · 2024-07-29T20:32:27Z

@lanesawyer, Thanks a ton for your work on the dark theme! Since a lot has changed since your PR, and there were too many conflicts, I created a new PR to pick up where you left off.

You can check it out here: Dark Mode Implementation

Really appreciate your contributions! 🎉🥳

lanesawyer · 2024-07-29T22:21:15Z

Woooo love to see it! Thanks for running with what I had and finishing it all up. That final PR is a big one!

Excited to start playing with the official dark mode once it's shipped 😄

Storybook dark mode plumbing in place

eb69b11

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/ButtonIcon/index.stories.tsx Outdated Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/LoadingIndicator/index.stories.tsx Outdated Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Outdated Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Outdated Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Outdated Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Outdated Show resolved Hide resolved

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Outdated Show resolved Hide resolved

Merge branch 'main' into hackathon/dark-mode

9152ddf

lanesawyer commented Nov 29, 2023

View reviewed changes

packages/components/src/core/styles/common/defaultTheme.ts Outdated Show resolved Hide resolved

lanesawyer added 2 commits November 29, 2023 13:09

Light and dark theme palette

eb7080f

Comments and TODOs

0e70163

lanesawyer commented Nov 29, 2023

View reviewed changes

lanesawyer and others added 5 commits November 29, 2023 13:47

Strongly type some objects, new TODO

589c5cd

Fix up reading from the theme in a storybook story

8166de1

Use MUI type for theme, remove mode from AppTheme

dcbdbfb

Merge branch 'main' into hackathon/dark-mode

b04c0e7

Light/dark backgrounds automatically in Storybook

cf8b16c

lanesawyer commented Nov 29, 2023

View reviewed changes

feat: update defaultTheme.ts colors

1e6b38f

- Updated DialogTitle to use palette.text colors - Updated Tabs to use palette.text colors

lanesawyer commented Nov 29, 2023

View reviewed changes

lanesawyer commented Nov 30, 2023

View reviewed changes

cathystoli added Design H1'24 labels Dec 5, 2023

masoudmanson added the Hackday label Dec 5, 2023

tihuan requested a review from masoudmanson January 19, 2024 21:24

tihuan self-requested a review January 19, 2024 21:24

tihuan reviewed Jan 19, 2024

View reviewed changes

Merge branch 'main' into hackathon/dark-mode

871bc04

lanesawyer commented Jan 19, 2024

View reviewed changes

Addressing PR feedback

1c71d4f

lanesawyer marked this pull request as ready for review January 19, 2024 22:41

tihuan approved these changes Jan 19, 2024

View reviewed changes

masoudmanson approved these changes Feb 1, 2024

View reviewed changes

masoudmanson mentioned this pull request Jul 29, 2024

feat(dark mode): Implementation #815

Merged

lanesawyer closed this Jul 29, 2024

masoudmanson added the Released label Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Dark Mode #718

feat: Dark Mode #718

lanesawyer commented Nov 29, 2023 •

edited

Loading

lanesawyer Nov 29, 2023

lanesawyer Nov 29, 2023

lanesawyer Jan 19, 2024

lanesawyer Nov 29, 2023

lanesawyer Nov 29, 2023

lanesawyer Nov 29, 2023

tihuan Jan 19, 2024

lanesawyer Nov 29, 2023

lanesawyer Nov 30, 2023 •

edited

Loading

tihuan Jan 19, 2024

lanesawyer Jan 19, 2024 •

edited

Loading

tihuan left a comment

tihuan Jan 19, 2024

tihuan Jan 19, 2024

lanesawyer Jan 19, 2024

tihuan Jan 19, 2024

tihuan Jan 19, 2024

lanesawyer Jan 19, 2024

hthomas-czi Feb 1, 2024

lanesawyer commented Jan 19, 2024 •

edited

Loading

tihuan left a comment

masoudmanson left a comment

masoudmanson commented Jul 29, 2024

lanesawyer commented Jul 29, 2024

feat: Dark Mode #718

feat: Dark Mode #718

Conversation

lanesawyer commented Nov 29, 2023 • edited Loading

Dark Mode

Remaining Work

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanesawyer Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanesawyer Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

tihuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanesawyer commented Jan 19, 2024 • edited Loading

tihuan left a comment

Choose a reason for hiding this comment

masoudmanson left a comment

Choose a reason for hiding this comment

masoudmanson commented Jul 29, 2024

lanesawyer commented Jul 29, 2024

lanesawyer commented Nov 29, 2023 •

edited

Loading

lanesawyer Nov 30, 2023 •

edited

Loading

lanesawyer Jan 19, 2024 •

edited

Loading

lanesawyer commented Jan 19, 2024 •

edited

Loading