-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix(button-toggle): long text overflows button and update large icon padding #6833
Conversation
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.
Really good stuff @nuria1110, my comments are non-blocking let me know what you think I'm happy to approve if you disagree 😄
small: 24, | ||
medium: 40, | ||
large: 48, | ||
small: "8px", |
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.
suggestion (non-blocking): might be worth using the tokens here, don't mind if it's a new bit of work as there's several config objects in this file that use hardcoded values
const paddingLargeIconConfig = {
small: "var(--spacing100)",
medium: "var(--spacing100) var(--spacing150) var(--spacing000)",
large: "var(--spacing100) var(--spacing300)",
};
@@ -378,11 +378,12 @@ export const DisabledGroup: Story = () => ( | |||
DisabledGroup.storyName = "Disabled Group"; | |||
|
|||
export const WrappedButtons: Story = () => ( | |||
<Box width="350px" display="flex" flexWrap="nowrap"> | |||
<Box width="375px" display="flex" flexWrap="nowrap"> |
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.
comment (non-blocking): we could set the width to be smaller when run in chromatic here so we get a snapshot of them stacked fully vertically
const inChromatic = isChromatic();
<Box width={inChromatic ? "175px" : "375px"} display="flex" flexWrap="nowrap">
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.
alternatively you could add a test story and capture the example you've screenshotted in the above PR description if you prefer that
…padding Fixes issue where longer text content overflows buttons when they wrap in smaller screens. Also updates button padding in buttons with large icons. fix #6799
🎉 This PR is included in version 141.0.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix #6799
Proposed behaviour
ButtonToggle
with a large icon is aligned with designs.ButtonToggle
with long text content wraps in smaller screens, the button's height is adjusted to fit the wrapped text and is centre aligned.Current behaviour
ButtonToggle
with a large icon is not aligned with designs.ButtonToggle
with long text content wraps in smaller screens, the text overflows the button and is not centre aligned.Checklist
d.ts
file added or updated if requiredQA
Additional context
Testing instructions