Skip to content
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: Dark Mode #718

Closed
wants to merge 12 commits into from
9 changes: 9 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,12 @@ storybook-static

# OS Files
.DS_Store

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

# Yarn 3 files
.pnp.*
.yarn/*
!.yarn/patches
!.yarn/plugins
!.yarn/releases
!.yarn/sdks
!.yarn/versions
37 changes: 31 additions & 6 deletions .storybook/preview.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
import { defaultTheme } from "../packages/components/src/core/styles";
import CssBaseline from "@mui/material/CssBaseline";
import { theme } from "../packages/components/src/core/styles";
import { ThemeProvider } from "@mui/material/styles";

export const decorators = [
(Story) => (
<ThemeProvider theme={defaultTheme}>
<Story />
</ThemeProvider>
),
(Story, context) => {
const { theme: storybookTheme } = context.globals;

return (
<ThemeProvider theme={theme(storybookTheme)}>
{/* CssBaseline provides light/dark background MUI theme for all stories */}
<CssBaseline />
<Story />
</ThemeProvider>
);
},
];

/**
Expand All @@ -15,8 +22,26 @@ export const decorators = [
* https://github.com/chromaui/storybook-addon-pseudo-states/issues/59#issuecomment-1498182067
*/
const preview = {
parameters: {
// Removes the change background button since it's controlled by the theme toggle
backgrounds: { disable: true },
},
globalTypes: {
pseudo: {},
theme: {
description: 'Global theme for components',
defaultValue: 'light',
toolbar: {
title: 'Theme',
icon: 'circlehollow',
dynamicTitle: true,
items: [
{ value: 'light', left: '☀️', title: 'Light mode' },
{ value: 'dark', left: '🌙', title: 'Dark mode' },
],
},
},
},
};

export default preview;
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixing a typo in the README


export const Tag = styled("div")`
// This is a callback function that returns more CSS rules, but the only way
Expand Down
10 changes: 5 additions & 5 deletions packages/components/src/core/Alert/index.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@ import { styled } from "@mui/material/styles";
import { Args, Meta } from "@storybook/react";
import React from "react";
import Button from "../Button";
import { defaultTheme } from "../styles/common/defaultTheme";
import Alert from "./index";

const DismissButton = styled(Button)`
margin-left: -${defaultTheme.spacing(3)}px;
const DismissButton = styled(Button)(
({ theme }) => `
margin-left: -${theme.spacing(3)}px;
padding-bottom: 0;
font-size: 12px;
line-height: 18px;
letter-spacing: 1px;
font-weight: 600;
&:hover {
background: none;
}
`;
}`
);

const Demo = (props: Args): JSX.Element => {
const { text } = props;
Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/core/Alert/style.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Alert } from "@mui/material";
import { styled } from "@mui/material/styles";
import { getColors, getShadows, getSpaces } from "../styles";
import { defaultTheme } from "../styles/common/defaultTheme";

export const StyledAlert = styled(Alert)`
${(props) => {
Expand All @@ -17,7 +16,7 @@ export const StyledAlert = styled(Alert)`
return `
background-color: ${backgroundColor};
margin: ${spacings?.m}px 0;
color: ${defaultTheme.palette.text.primary};
color: ${props.theme.palette.text.primary};
padding: ${spacings?.l}px ${spacings?.l}px
${spacings?.l}px 9px;
background-color: ${alertColor};
Expand Down
6 changes: 4 additions & 2 deletions packages/components/src/core/ButtonIcon/index.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { useTheme } from "@mui/material/styles";
import { Args, Meta } from "@storybook/react";
import React from "react";
import { defaultAppTheme } from "../styles";
import { SDSTheme } from "../styles";
import RawButtonIcon from "./index";
import { ButtonIconExtraProps, ButtonIconSizeToTypes } from "./style";

Expand Down Expand Up @@ -95,7 +96,8 @@ export const Default = {
// Live Preview

const LivePreviewDemo = (): JSX.Element => {
const spacings = defaultAppTheme?.spacing;
const theme: SDSTheme = useTheme();
const spacings = theme.app?.spacing;

const livePreviewStyles: React.CSSProperties = {
alignItems: "center",
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/core/Callout/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
getIconSizes,
getSpaces,
} from "../styles";
import { defaultTheme } from "../styles/common/defaultTheme";

interface CalloutExtraProps extends CommonThemeProps {
collapsed?: boolean;
Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type issue, needs to be addressed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return `
background-color: ${backgroundColor};
width: 360px;
margin: ${spacings?.m}px 0;
border-radius: ${corners?.m}px;
color: ${defaultTheme.palette.text.primary};
color: ${props.theme?.palette.text.primary};
padding: ${spacings?.m}px;
background-color: ${calloutColor};

Expand Down
8 changes: 6 additions & 2 deletions packages/components/src/core/DialogTitle/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ export const StyledDialogTitle = styled(DialogTitle)`
export const Title = styled(Typography)`
${fontHeaderXl}

color: black;
${(props) => {
return `
color: ${props.theme.palette.text.primary};
Copy link
Author

@lanesawyer lanesawyer Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍 !

Copy link
Author

@lanesawyer lanesawyer Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

`;
}}
`;

export const Subtitle = styled(Typography)`
Expand All @@ -34,7 +38,7 @@ export const Subtitle = styled(Typography)`
const colors = getColors(props);

return `
color: ${colors?.gray[500]};
color: ${props.theme.palette.text.secondary};
`;
}}
`;
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { useTheme } from "@mui/material/styles";
import { Args, Meta } from "@storybook/react";
import React from "react";
import { defaultAppTheme } from "../styles";
import { SDSTheme } from "../styles";
import RawLoadingIndicator from "./index";

const LoadingIndicator = (props: Args): JSX.Element => {
Expand Down Expand Up @@ -37,7 +38,8 @@ export const CustomAriaLabel = {
// Live Preview

const LivePreviewDemo = (props: Args): JSX.Element => {
const spacings = defaultAppTheme?.spacing;
const theme: SDSTheme = useTheme();
const spacings = theme.app?.spacing;

const livePreviewStyles = {
display: "grid",
Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/core/Notification/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
getShadows,
getSpaces,
} from "../styles";
import { defaultTheme } from "../styles/common/defaultTheme";

const fontBodyXs = fontBody("xs");

Expand All @@ -33,7 +32,7 @@ export const StyledNotification = styled(Alert)`
box-sizing: border-box;
margin: ${spacings?.m}px 0;
border-radius: ${corners?.m}px;
color: ${defaultTheme.palette.text.primary};
color: ${props.theme.palette.text.primary};
padding: ${spacings?.l}px;
align-items: flex-start;
background-color: ${notificationColor};
Expand Down
16 changes: 9 additions & 7 deletions packages/components/src/core/Tabs/style.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nice!

};
`;
}}
`;
Expand Down Expand Up @@ -89,26 +91,26 @@ export const StyledTab = styled(RawTab, {
// (thuang): Large Tab height is 30px, the offset is 4px
height: ${isLarge ? 26 : 22}px;

color: ${colors?.gray[500]};
color: ${props.theme.palette.text.secondary};

&:hover, :focus {
color: black;
color: ${props.theme.palette.text.primary};
}

&.Mui-selected {
color: black;
color: ${props.theme.palette.text.primary};

&:hover {
color: black;
color: ${props.theme.palette.text.primary};
}
}

&:active {
color: black;
color: ${props.theme.palette.text.primary};
}

&:disabled {
color: ${colors?.gray[200]};
color: ${props.theme.palette.text.disabled};
}
`;
}}
Expand Down
Loading