Repository navigation
fix(badge): ensure fullWidth Button children render correctly - #8229
Conversation
| ${$fullWidth && | ||
| css` | ||
| width: 100%; | ||
| [data-role="badge-wrapper"]:has(> &) { |
There was a problem hiding this comment.
suggestion: could we apply a data- tag on Button when fullWidth is applied and then add the width in Badge instead?
| chromatic: { disableSnapshot: false }, | ||
| }; | ||
|
|
||
| export const WithFullWidthButtons: Story = () => { |
There was a problem hiding this comment.
suggestion: Sorry I missed this first time around but it looks like this story we've added won't trigger a snapshot.
In the root of the file we set { disableSnapshot: true } and don't override it here. I think we need to override it here to make sure the snapshot is triggered.
This may be slightly out of scope but you could count up the stories which have { disableSnapshot: false } on them, if there are more stories being overriden than not you could flip the logic in the file.
There was a problem hiding this comment.
Hi, I've made the suggested changes and updated the PR. Could I get another review please? Thanks :)
DipperTheDan
left a comment
DipperTheDan
left a comment
There was a problem hiding this comment.
Happy to approve once the comment about the Chromatic snapshot by @tomdavies73 has been addressed 👍🏻
| padding: "var(--global-space-layout-xs)", | ||
| backgroundColor: "rgb(242, 245, 246)", | ||
| }; | ||
| // import { Sizes } from "./badge.stories"; |
There was a problem hiding this comment.
nitpick: Commented out code has been left in here.
| // import { Sizes } from "./badge.stories"; |
There was a problem hiding this comment.
Ah thank you for spotting this! I will remove that now
paulrobinson88
left a comment
paulrobinson88
left a comment
There was a problem hiding this comment.
Hi Kaitlyn, nice work on this, just the one comment from me, all looks good otherwise.
| }; | ||
|
|
||
| const meta: Meta<typeof Badge> = { | ||
| title: "Badge/Test", |
There was a problem hiding this comment.
nitpick: As we have enabled the snapshot in this file, can we disable the none theme to just leave us with the sage one as it's not really needed?
We should be able to add a line similar to themeProvider: { chromatic: { theme: "sage" } },
You can see the pattern of this in the pager-test.stories.tsx
109d487
540dffe
fixed issue where fullWidth Button children don't extend their full width when wrapped in a badge fix #8202
392c6b9
|
🎉 This PR is included in version 162.16.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
fix #8202
Proposed behaviour
fixed issue where fullWidth Button children don't extend there full width when wrapped in a badge
Current behaviour
fullWidth Buttons' layouts break when used in Badge
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
see WithFullWidthButtons for fix