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
+49 −23
Closed

Add button color prop and new error style button #111

Show file tree
Hide file tree
Changes from 2 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 = RawButtonProps & 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
39 changes: 30 additions & 9 deletions src/core/Button/style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import styled from "@emotion/styled";
import { Button } from "@material-ui/core";
import { Button, ButtonProps } from "@material-ui/core";
import {
fontCapsXxxs,
getColors,
Expand All @@ -8,16 +8,31 @@ import {
Props,
} from "../styles";

const sdsPropNames = ["isAllCaps", "isRounded", "sdsStyle", "sdsType"];
export interface ExtraProps {
isAllCaps?: boolean;
isRounded?: boolean;
sdsStyle?: "minimal" | "rounded" | "square";
sdsType?: "primary" | "secondary";
color?: "primary" | "secondary" | "success" | "error" | "warning" | "info";
}

// 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 colors = getColors(props);
const spacings = getSpaces(props);

Expand All @@ -30,13 +45,19 @@ const ButtonBase = styled(Button, {

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

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 (!color) {
throw new Error(`Button color not found: ${colorProp}`);
}

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

Choose a reason for hiding this comment

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

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 🙆‍♂️

box-shadow: none;
}
&:focus {
Expand All @@ -45,7 +66,7 @@ const ButtonBase = styled(Button, {
}
&:active {
color: white;
background-color: ${colors?.primary[600]};
background-color: ${color[600]};
box-shadow: none;
}
&:disabled {
Expand Down Expand Up @@ -75,7 +96,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 +165,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