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
30 changes: 24 additions & 6 deletions .storybook/preview.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { defaultTheme } from "../packages/components/src/core/styles";
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)}>
<Story />
</ThemeProvider>
);
},
];

/**
Expand All @@ -17,6 +21,20 @@ export const decorators = [
const preview = {
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
4 changes: 2 additions & 2 deletions packages/components/src/core/ButtonIcon/index.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Args, Meta } from "@storybook/react";
import React from "react";
import { defaultAppTheme } from "../styles";
import { sharedAppTheme } from "../styles";
import RawButtonIcon from "./index";
import { ButtonIconExtraProps, ButtonIconSizeToTypes } from "./style";

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

const LivePreviewDemo = (): JSX.Element => {
const spacings = defaultAppTheme?.spacing;
const spacings = sharedAppTheme?.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
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 @@ -30,7 +31,8 @@ export const Default = {
// 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
Loading