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

Jump to bottom

feat(button): add v5 color support #113

Closed
wants to merge 4 commits into from
Closed

feat(button): add v5 color support #113

wants to merge 4 commits into from

Conversation

tihuan
Copy link
Contributor

@tihuan tihuan commented Feb 5, 2022 •
edited
Loading

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

This ticket adds MUI button v5 color support, which expands v4's options "primary", "secondary", to color?: "primary" | "secondary" | "success" | "error" | "warning" | "info";

Implementation inspiration from: mui/material-ui#13875 (comment)

demo

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 tihuan force-pushed the thuang-button-color-test branch 2 times, most recently from 6648d55 to 1923944 Compare February 5, 2022 11:11
@tihuan tihuan mentioned this pull request Feb 5, 2022
Closed
7 tasks
@tihuan tihuan force-pushed the thuang-button-color-test branch 2 times, most recently from 7612b25 to 9c54a91 Compare February 5, 2022 11:18
@tihuan tihuan requested a review from jfoo1984 February 5, 2022 11:19
@tihuan tihuan force-pushed the thuang-button-color-test branch 7 times, most recently from e1157ba to 001f8e4 Compare February 5, 2022 13:00
jfoo1984
jfoo1984 reviewed Feb 5, 2022
View reviewed changes
src/core/Button/style.ts Outdated
type V5ButtonProps = Props & ExtraProps & Omit<ButtonProps, "color">;

// Please keep this in sync with the props used in `ExtraProps`
const doNotForwardProps = [
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we could rename this to nonMuiProps or something along those lines? That is a little more expressive around why we don't want to forward these props. (I'm happy to make this change but wanted to run it by you first).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, great point! Updated 😄 Thank you!

@jfoo1984
Copy link
Contributor

jfoo1984 commented Feb 5, 2022 •
edited
Loading

I like this approach and how the styles function more clearly encapsulate different styles and allow us to reuse the function from one place! Thank you for putting this together!

One general comment (and I'm happy to make this change), for the "external" interface for a button (provided in src/core/Button/index.tsx), could we exclude color and variant? I think that in the switch statement, we're basically using sdsStyle and sdsType to determine what to pass for those values into styles.ts. By exposing them externally, we're possibly allowing someone to use different combinations for those that haven't been specified in Storybook (and it seems like one of the major benefits of having a design system is to restrict the variants that are used in the app). Any thoughts on that?

@jfoo1984
Copy link
Contributor

jfoo1984 commented Feb 5, 2022 •
edited
Loading

This is a follow-on to the last comment. I think we don't actually need to pass sdsStyle as a prop into styles.ts. The only thing we do with sdsStyle is prevent it from being forwarded in BaseButton to the underlying MUI button. sdsType has one small usage in styles.ts, but it might actually be nice if we can find a to refactor that out too.

In conjunction with my last comment, I feel like if we can do this, we would have each layer encapsulated pretty nicely. so the external interface is just a user specifies a sdsType and sdsStyle, the index.js processes those settings into the right values to pass along for color and variant, as well as choosing the right underlying button component type, and then styles.ts is just strictly using those values to generate the right MUI component.

So for adding an error button style, the external interface would be to pass in sdsType = "error", then within index.js, we can translate that to the appropriate color and variant to pass on to style.ts to set up the MUI button.

There is one potential gotcha I can see with this (which I think we talked about before in Slack). Currently, we only have a contained error button style, so this solution works fine. If for some reason, we needed both a contained and outlined error button style in the future, we'll have to either add a different sdsType or some other external parameter. But the positive side of that is that it allows us to define our own language for the SDS that is separate from what MUI provides.

Sorry for another super long writeup on this 😅. I'm definitely open to just getting on a call to talk through any of this if that's easier and happy to tackle the stuff that I've mentioned here!

@SDS-Design
Copy link

SDS-Design commented Feb 7, 2022 via email

@tihuan
Copy link
Contributor Author

tihuan commented Feb 7, 2022

WTF happened here?

Woah yeah 🤯 How did you get the notification? Did we accidentally tag you somehow??

@tihuan
Copy link
Contributor Author

tihuan commented Feb 7, 2022

I like this approach and how the styles function more clearly encapsulate different styles and allow us to reuse the function from one place! Thank you for putting this together!

One general comment (and I'm happy to make this change), for the "external" interface for a button (provided in src/core/Button/index.tsx), could we exclude color and variant? I think that in the switch statement, we're basically using sdsStyle and sdsType to determine what to pass for those values into styles.ts. By exposing them externally, we're possibly allowing someone to use different combinations for those that haven't been specified in Storybook (and it seems like one of the major benefits of having a design system is to restrict the variants that are used in the app). Any thoughts on that?

Thanks so much for the thoughtful comment 🤩 🙏 !

Yeah, exposing MUI props at the top level is intentional, because our SDS components need to provide the flexibility (and escape hatches) for any product to overwrite the style as needed. The reason is because SDS aims to provide reasonable defaults as recommendations (instead of prescriptions), but ultimately the product team should retain the control of how they want to style their components. This will continue to be needed as external partners start using SDS as well 🙆‍♂️

@tihuan tihuan force-pushed the thuang-button-color-test branch 3 times, most recently from 85eddad to 6192105 Compare February 7, 2022 22:05
@tihuan
Copy link
Contributor Author

tihuan commented Feb 7, 2022

@jfoo1984 Thanks again for all the great ideas and feedback 🎉 🤩 🙏 I've updated the PR according to our discussion this morning. PTAL thank you!

@SDS-Design
Copy link

SDS-Design commented Feb 8, 2022 via email

@tihuan
Copy link
Contributor Author

tihuan commented Feb 8, 2022

@SDS-Design That's amazing! Really nice meeting you too, Sandra 😂 🎉 Hopefully you get to use this component library down the road 🤩 👏

Have an amazing day!

My friend, I have here the full thread and you named it with SDS and sds design, that are my initials, Sandra Diana Schöner. I'm programmer but... Not long, since 3 years and you name a function after me, WTF 🤣. Thank you for that honor 🙏. It's angular, I understand it, I think one day I opened an issue and yeah. Here we are. SDS
…

tihuan
tihuan commented Feb 17, 2022
View reviewed changes
src/core/Button/style.ts
isAllCaps?: boolean;
isRounded?: boolean;
/**
* TODO(185930): Remove custom `color` prop when we upgrade to MUIv5.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdjmoore I forgot we haven't merged the button PR 😂

Also created a ticket already!

https://app.shortcut.com/sci-design-system/story/185930/material-ui-mui-v5-todos

Thank you!

@tihuan tihuan marked this pull request as ready for review February 22, 2022 23:35
mdjmoore
mdjmoore approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@mdjmoore mdjmoore left a comment

Choose a reason for hiding this comment

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

┗(＾0＾)┓

@tihuan
Copy link
Contributor Author

tihuan commented Mar 9, 2022

I'll double check this PR addresses specs from https://app.shortcut.com/sci-design-system/story/188176/implement-error-variant-on-button-component !

@tihuan
Copy link
Contributor Author

tihuan commented Dec 14, 2022

No longer needed, since MUIv5 migration is done!

@tihuan tihuan closed this Dec 14, 2022
@tihuan tihuan deleted the thuang-button-color-test branch December 14, 2022 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfoo1984 jfoo1984 jfoo1984 left review comments

@mdjmoore mdjmoore mdjmoore approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tihuan @jfoo1984 @SDS-Design @mdjmoore