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.

Sign up for GitHub

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

Add button color prop and new error style button #111

Closed
wants to merge 6 commits into from
Closed

Add button color prop and new error style button #111

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
125b7e3
idea: button color prop
tihuan Jan 29, 2022
70a3f68
update ts
tihuan Jan 29, 2022
fd8b927
Fix ts issue
jfoo1984 Jan 31, 2022
a356661
Add error border style for error 600
jfoo1984 Jan 31, 2022
a8a05c8
Button style updates
jfoo1984 Jan 31, 2022
6e0b888
Merge branch 'main' into thuang-button-color
jfoo1984 Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions src/core/Button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,19 @@
import { ButtonProps as RawButtonProps } from "@material-ui/core";
import React from "react";
import {
ExtraProps,
PrimaryMinimalButton,
RoundedButton,
SecondaryMinimalButton,
SquareButton,
StyledButton,
} from "./style";

interface SdsProps {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving this into style.ts as ExtraProps

isAllCaps?: boolean;
isRounded?: boolean;
sdsStyle?: "minimal" | "rounded" | "square";
sdsType?: "primary" | "secondary";
}
export type ButtonProps = RawButtonProps & SdsProps;
export type ButtonProps = Omit<RawButtonProps, "color"> & ExtraProps;

const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
(props: ButtonProps, ref): JSX.Element | null => {
const sdsStyle = props?.sdsStyle;
const sdsType = props?.sdsType;
const { color = "primary", sdsStyle, sdsType } = props;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since MUI Button already accepts color prop, we default the color selection to "primary" since SDS only uses the primary color most of the time


if (!sdsStyle || !sdsType) {
// eslint-disable-next-line no-console
Expand All @@ -44,7 +38,7 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
case sdsStyle === "rounded" && sdsType === "primary":
return (
<RoundedButton
color="primary"
color={color}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we pass the color to each styled button!

ref={ref}
variant="contained"
{...propsWithDefault}
Expand All @@ -53,7 +47,7 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
case sdsStyle === "rounded" && sdsType === "secondary":
return (
<RoundedButton
color="primary"
color={color}
ref={ref}
variant="outlined"
{...propsWithDefault}
Expand All @@ -62,7 +56,7 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
case sdsStyle === "square" && sdsType === "primary":
return (
<SquareButton
color="primary"
color={color}
ref={ref}
variant="contained"
{...propsWithDefault}
Expand All @@ -71,7 +65,7 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
case sdsStyle === "square" && sdsType === "secondary":
return (
<SquareButton
color="primary"
color={color}
ref={ref}
variant="outlined"
{...propsWithDefault}
Expand All @@ -80,7 +74,7 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
case sdsStyle === "minimal" && sdsType === "primary":
return (
<PrimaryMinimalButton
color="primary"
color={color}
ref={ref}
variant="text"
{...propsWithDefault}
Expand Down
49 changes: 40 additions & 9 deletions src/core/Button/style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,40 @@
import styled from "@emotion/styled";
import { Button } from "@material-ui/core";
import { Button, ButtonProps } from "@material-ui/core";
import {
fontCapsXxxs,
getBorders,
getColors,
getCorners,
getSpaces,
Props,
} from "../styles";

const sdsPropNames = ["isAllCaps", "isRounded", "sdsStyle", "sdsType"];
export interface ExtraProps {
color?: "primary" | "error";
Copy link
Contributor

@jfoo1984 jfoo1984 Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@tihuan I changed this to only accept the colors that we have used / defined for buttons. For reference, this is what is was before:

  color?: "primary" | "secondary" | "success" | "error" | "warning" | "info";

The problem I was running into was that some of the colors aren't defined in our theme at all, or they're missing certain various (400, 600, etc.).

However, this brings up bit of a philosophical question here. Are we ok with just defining components that we need as we go (essentially the approach taken here, since I'm mainly working on this to add the error style), which keeps the SDS to the minimal set of components that we actually use? Or do we want to flesh out all the possible color values for buttons that could be used in MUI so that they will be available in the future if someone needed them (so this would involve having the whole list of color and making sure they work, and probably adding them in storybook)?

Copy link

Choose a reason for hiding this comment

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

Hey Jerry! I want to jump in here—our philosophy with the SDS is for it to be opinionated and specific, meaning we only plan on adding components and their variants as we see the need arise, this is what helps distinguish our design system from simply becoming a sprawling component library. There are a few instances where we have deviated from that (for example anticipating the need for a primary square dropdown button, even though none of our products currently use it, it seemed like there was enough of a potential need to just create it), but overall our approach is to define components that we need as we go. Lmk if you've got any questions!

Copy link
Contributor

@jfoo1984 jfoo1984 Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for clarifying @clarsen-czi! That makes sense and seems like the right approach to me, to just add what we need as we go instead of defining a broad set of components that may never be used.

In the context of this PR, since we currently only use primary and error colors on buttons, we probably don't want to define the whole set of colors available in MUI then, as that backdoors us into having some colors and border styles that we don't currently use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey all! Sorry for the late reply 😆 On PTO today!

Yeah I think limiting the color value scope sounds great and matches the actual behavior 👍

Thanks for updating that!

Copy link
Contributor

Choose a reason for hiding this comment

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

@tihuan Get out of here! You're on PTO! 🤣

isAllCaps?: boolean;
isRounded?: boolean;
sdsStyle?: "minimal" | "rounded" | "square";
sdsType?: "primary" | "secondary";
}

// Please keep this in sync with the props used in `ExtraProps`
const doNotForwardProps = [
"isAllCaps",
"isRounded",
"sdsStyle",
"sdsType",
"color",
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so I think not forwarding color to the MUI button is causing issues. If you look at what was previously on line 11:

const sdsPropNames = ["isAllCaps", "isRounded", "sdsStyle", "sdsType"];

we did not have color so we did forward it. I'm beginning to think that the code reuse we're trying to achieve here may not be sustainable in some ways. I'll start a comment on the overall PR about that.

];

const ButtonBase = styled(Button, {
shouldForwardProp: (prop) => {
return !sdsPropNames.includes(prop.toString());
return !doNotForwardProps.includes(String(prop));
},
})`
box-shadow: none;
${(props) => {
const { variant } = props;
${(props: Props & ExtraProps & Omit<ButtonProps, "color">) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the TS issue!

const { variant, color: colorProp = "primary" } = props;
const borders = getBorders(props);
const colors = getColors(props);
const spacings = getSpaces(props);

Expand All @@ -30,13 +47,26 @@ const ButtonBase = styled(Button, {

const padding = variant === "outlined" ? outlinedPadding : containedPadding;

const border = borders && borders[colorProp];
const color = colors && colors[colorProp];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we pick the color object based on colorProp, which defaults to "primary" from index.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I haven't fixed the type error here 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! https://github.com/chanzuckerberg/sci-components/pull/111/files#r794982993


if (!border) {
throw new Error(`Button border not found: ${colorProp}`);
}
if (!color) {
throw new Error(`Button color not found: ${colorProp}`);
}

return `
background-color: ${color[400]};
border: ${border[400]};
color: white;
Comment on lines +61 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

@tihuan, adding these attributes seems to be breaking the secondary button, as that is no longer styled correctly (it ends up getting filled so it just looks like the primary button). I think we may need to go with just creating a separate override style for the error button. I don't know if I'll have time to give that a shot, but maybe we can sync up early next week?

padding: ${padding};
min-width: 120px;
height: 34px;
&:hover, &:focus {
color: white;
background-color: ${colors?.primary[500]};
background-color: ${color[600]};
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no 500 defined for error color, so I just changed this to use 600 for any color. The other option would be to add a 500 color for error. Note that this makes the hover color the same as active below (which was previously 600).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh thanks for pointing this out!

@clarsen-czi @hthomas-czi Should we define error 500 value in this case?

Thanks all!

Copy link
Contributor

@hthomas-czi hthomas-czi Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the ideal behavior would be to use 500 if that value exists, otherwise use 600. Then all colors should have a hover state and primary will have a separate hover and active states.

Copy link

Choose a reason for hiding this comment

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

I agree with Harley—I think we'd ideally use error500 which doesn't exist. I've just reached out to the designers on the design system team Slack to make sure they're ok with us adding it in. I'll follow up once I hear back

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 chiming in! adding error500 sounds like a good path. Then we can consistently have separate hover and active colors for all button types. I'm happy to add in the error500 color once we've got it!

Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Add error500 color (#C61128), and use 500 color for hover.

https://czi-sci.slack.com/archives/C029M3YCFFD/p1643844470244839?thread_ts=1643674010.289539&cid=C029M3YCFFD

box-shadow: none;
}
&:focus {
Expand All @@ -45,7 +75,8 @@ const ButtonBase = styled(Button, {
}
&:active {
color: white;
background-color: ${colors?.primary[600]};
background-color: ${color[600]};
border: ${border[600]};
box-shadow: none;
}
&:disabled {
Expand Down Expand Up @@ -75,7 +106,7 @@ interface IsAllCaps extends Props {

const MinimalButton = styled(Button, {
shouldForwardProp: (prop) => {
return !sdsPropNames.includes(prop.toString());
return !doNotForwardProps.includes(String(prop));
},
})`
${(props: IsAllCaps) => {
Expand Down Expand Up @@ -144,7 +175,7 @@ interface IsRounded extends Props {
}
export const StyledButton = styled(Button, {
shouldForwardProp: (prop) => {
return !sdsPropNames.includes(prop.toString());
return !doNotForwardProps.includes(String(prop));
},
})`
&:focus {
Expand Down
1 change: 1 addition & 0 deletions src/core/styles/common/defaultTheme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ const defaultThemeColors = {
const borders = {
error: {
"400": `1px solid ${defaultThemeColors.error["400"]}`,
"600": `1px solid ${defaultThemeColors.error["600"]}`,
},
gray: {
"100": `1px solid ${defaultThemeColors.gray["100"]}`,
Expand Down