Add button color prop and new error style button #111

tihuan · 2022-01-29T01:02:48Z

title: "type_of_change(scope_of_work): ticket title"
reviewers: "sds-eng, sds-design"

Summary

Structural Element (Base, Gene, DNA, Chromosome or Cell)
Shortcut ticket: sh-XXXX
Copy ticket descirption here

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 @SDS-Design

tihuan · 2022-01-29T01:03:06Z

src/core/Button/index.tsx

  PrimaryMinimalButton,
  RoundedButton,
  SecondaryMinimalButton,
  SquareButton,
  StyledButton,
 } from "./style";

-interface SdsProps {


moving this into style.ts as ExtraProps

tihuan · 2022-01-29T01:04:17Z

src/core/Button/index.tsx


 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;


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

tihuan · 2022-01-29T01:04:31Z

src/core/Button/index.tsx

@@ -44,7 +38,7 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
      case sdsStyle === "rounded" && sdsType === "primary":
        return (
          <RoundedButton
-            color="primary"
+            color={color}


we pass the color to each styled button!

tihuan · 2022-01-29T01:06:19Z

src/core/Button/style.ts

@@ -30,13 +45,15 @@ const ButtonBase = styled(Button, {

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

+    const color = colors && colors[colorProp];


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

also I haven't fixed the type error here 😂

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

tihuan · 2022-01-29T01:08:02Z

src/core/Button/style.ts

    return `
      padding: ${padding};
      min-width: 120px;
      height: 34px;
      &:hover, &:focus {
        color: white;
-        background-color: ${colors?.primary[500]};
+        background-color: ${color[500]};


once we upgrade to MUI v5, which supports different color value other than primary and secondary, we can remove the custom css in SDS Button that v5 now does for us, and the change will be transparent to SDS library user, because the API is the same 🙆‍♂️

tihuan · 2022-01-29T01:58:20Z

src/core/Button/style.ts

  },
 })`
  box-shadow: none;
-  ${(props) => {
-    const { variant } = props;
+  ${(props: Props & ExtraProps & Omit<ButtonProps, "color">) => {


This fixes the TS issue!

jfoo1984 · 2022-01-31T17:37:32Z

src/core/Button/style.ts

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


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).

Ahh thanks for pointing this out!

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

Thanks all!

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.

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

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!

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

jfoo1984 · 2022-01-31T17:41:56Z

src/core/Button/style.ts

  getColors,
  getCorners,
  getSpaces,
  Props,
 } from "../styles";

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


@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)?

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!

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.

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!

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

jfoo1984 · 2022-02-04T23:17:14Z

src/core/Button/style.ts

+      background-color: ${color[400]};
+      border: ${border[400]};
+      color: white;


@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?

jfoo1984 · 2022-02-04T23:31:33Z

src/core/Button/style.ts

+  "isRounded",
+  "sdsStyle",
+  "sdsType",
+  "color",


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.

jfoo1984 · 2022-02-05T00:03:18Z

So, I'm having some concerns around this approach where we are trying to generalize the code around colors. I'm running into some conflicts between the secondary and error styles that can't seem to be resolved. Basically, to get the error button to display correctly, I need to add this CSS to the base button:

      background-color: ${color[400]};
      border: ${border[400]};
      color: white;

Those attributes weren't set before. I believe we were basically relying on passing color to MUI, and allowing their defined primary and secondary to set those attributes. But when I set those attributes, it breaks the secondary button (it basically turns it into the primary button).

The other thing that breaks the secondary button is that we have to pass through the color prop to MUI as well, because we were relying on styling from that.

Due to these issues, I don't think we can generalize the base button code to support the new error type, and I think it may be easier and clearer to just add error button blocks in the switch statement in src/core/Button/index.tsx.

However, that leads to some other issues, and we'd have to check specifically for color in addition to sdsType and sdsStyle, and make sure it's done in the right order so it doesn't fall through to the wrong part of the switch statement. I'm thinking that we actually should just add error as an sdsType option, but the switch statement can create it with the needed style

tihuan · 2022-02-05T11:11:31Z

Hey Jerry! Thanks so much for your detailed investigation and writeup 🙏 Updating the button to support more color variants is definitely way more work than expected 😭

I happened to find a thread talking about the same problem and their solution seems to work for us too!

I opened a new draft PR to test it out 🙆‍♂️

PTAL thank you!

jfoo1984 · 2022-02-05T21:15:53Z

Thanks for taking a look and putting the other PR together! I'll check it out!

tihuan · 2022-02-22T23:35:55Z

Real PR: #113

idea: button color prop

125b7e3

tihuan force-pushed the thuang-button-color branch from 11af1da to 125b7e3 Compare January 29, 2022 01:05

tihuan commented Jan 29, 2022

View reviewed changes

update ts

70a3f68

tihuan commented Jan 29, 2022

View reviewed changes

jfoo1984 added 3 commits January 31, 2022 09:32

Fix ts issue

fd8b927

Add error border style for error 600

a356661

Button style updates

a8a05c8

jfoo1984 reviewed Jan 31, 2022

View reviewed changes

Merge branch 'main' into thuang-button-color

6e0b888

jfoo1984 changed the title ~~idea: button color prop~~ Add button color prop and new error style button Feb 4, 2022

jfoo1984 reviewed Feb 4, 2022

View reviewed changes

tihuan closed this Feb 22, 2022

mmmmmmaya deleted the thuang-button-color branch May 5, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button color prop and new error style button #111

Add button color prop and new error style button #111

tihuan commented Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

tihuan Jan 29, 2022

jfoo1984 Jan 31, 2022

tihuan Jan 31, 2022

hthomas-czi Jan 31, 2022 •

edited

Loading

clarsen-czi Feb 1, 2022

jfoo1984 Feb 1, 2022

jfoo1984 Feb 2, 2022

jfoo1984 Jan 31, 2022 •

edited

Loading

clarsen-czi Jan 31, 2022

jfoo1984 Jan 31, 2022 •

edited

Loading

tihuan Jan 31, 2022

hthomas-czi Feb 1, 2022

jfoo1984 Feb 4, 2022

jfoo1984 Feb 4, 2022

jfoo1984 commented Feb 5, 2022

tihuan commented Feb 5, 2022

jfoo1984 commented Feb 5, 2022

tihuan commented Feb 22, 2022

		@@ -30,13 +45,15 @@ const ButtonBase = styled(Button, {

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

		const color = colors && colors[colorProp];

Add button color prop and new error style button #111

Add button color prop and new error style button #111

Conversation

tihuan commented Jan 29, 2022

title: "type_of_change(scope_of_work): ticket title" reviewers: "sds-eng, sds-design"

Summary

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hthomas-czi Jan 31, 2022 • edited Loading

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

jfoo1984 Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfoo1984 Jan 31, 2022 • edited Loading

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

jfoo1984 commented Feb 5, 2022

tihuan commented Feb 5, 2022

jfoo1984 commented Feb 5, 2022

tihuan commented Feb 22, 2022

title: "type_of_change(scope_of_work): ticket title"
reviewers: "sds-eng, sds-design"

hthomas-czi Jan 31, 2022 •

edited

Loading

jfoo1984 Jan 31, 2022 •

edited

Loading

jfoo1984 Jan 31, 2022 •

edited

Loading