-
Notifications
You must be signed in to change notification settings - Fork 85
feat(button): aligns Button with the new DS styling and functionality #7624
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
base: master
Are you sure you want to change the base?
Conversation
f9d2a76 to
292c698
Compare
nuria1110
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @damienrobson-sage, I think we need to look into the fallbacks it's a bit up in the air right now we're trying to decide if we need them at all and if we do should they just fallback to the legacy ones, to me hardcoding them feels redundant as it negates the point of the tokens and the wrapper. I'll let you know where we get with that so I've not commented on them specifically for now.
I also think the commit message could do with some tweaks, currently the release note will just say aligns Button with the new DS styling and functionality. I think it's worth being a bit more descriptive in of what's changed, that may well need to be in a commit message comment that we copy into the release notes but I think it will be more helpful than the current. It also might be worth splitting the adding of the new and deprecating the old into separate commits.
eb0a198 to
38e47fd
Compare
758b97a to
7177646
Compare
424cf5f to
84780a8
Compare
932160b to
5d51446
Compare
c6735d0 to
3b8fc67
Compare
ljemmo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @edleeks87 - left a couple of comments inline where layout spacing tokens were used inline instead of comp spacing tokens.
Additionally, i found these other elements to consider:
- When a button is composed with an icon, it appears we are consuming old tokens instead of new:
- Icon only button example is not a circle, it should have the same width as height. Figma link here.
- Inverse only has 1 example (primary button). For completeness it might be useful to show all the other inverse options in Storybook for devs to understand what they would look like:
- I would expect the
cursor: not-allowed;treatment to be applied to disabled button but currently there's no cursor change.
| height: "var(--global-size-m)", | ||
| iconOnlyWidth: "var(--global-size-m)", | ||
| paddingVertical: "6px", | ||
| paddingHorizontal: "var(--global-space-layout-2-xs)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edleeks87 the spacing token consumed here says layout. These are reserved for layout stacks such as groups of tiles or buttons. Instead the token consumed should be --global-space-comp-L for 16px left and right padding.
| height: "var(--global-size-l)", | ||
| iconOnlyWidth: "var(--global-size-l)", | ||
| paddingVertical: "6px", | ||
| paddingHorizontal: "var(--global-space-layout-2-xs)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edleeks87 Large size should consume --global-space-comp-L also
Introduce the "next" version of Button that consumers can start to
import and use in place of the existing Button implementation
- Deprecates darkBackground, gradient-grey and gradient-white
ButtonType from current button implementation
- Deprecates the old button entirely to ensure full awareness
- Deprecates the following properties from the old component to
indicate future removal:
- isWhite
- destructive
- subtext
- buttonType
fcdf440 to
62d5492
Compare
clairedenning
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed Luke's requests and the subsequent changes, and everything looks good to me!

Proposed behaviour
ButtonimplementationdarkBackground,gradient-greyandgradient-whiteButtonType from current button implementationisWhiteprop from current button implementationdestructiveprop from current button implementationsubtextprop from current button implementationbuttonTypeprop from current button implementationCurrent behaviour
The
Buttoncomponent is not aligned with the latest DS requirements.Checklist
d.tsfile added or updated if requiredQA
Additional context
Deprecatedsection of Storybook and points users towards the newer documentationTesting instructions
Use the new Storybook documentation to check the new functionality