Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Pull request overview
Adds support for rendering Button with variant="tag" at the smaller Tag size, aligning Button’s tag variant behavior with the existing Tag component sizing API (Issue #1957).
Changes:
- Added a
sizeprop toButtonandTagButtonand appliedTag--smallstyling whenvariant="tag"andsize="small". - Added unit tests covering
size="small"for bothButton(tag variant) andTagButton. - Added Playwright component screenshot coverage and committed new baseline images for small tag buttons (light/dark).
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/components/TagButton/index.tsx | Plumbs size through TagButton into Button. |
| packages/react/src/components/TagButton/index.test.tsx | Adds a unit test asserting Tag--small is applied. |
| packages/react/src/components/Button/index.tsx | Introduces size prop and toggles Tag--small class for tag variant. |
| packages/react/src/components/Button/index.test.tsx | Adds unit test for small tag button class behavior. |
| packages/react/src/components/Button/screenshots.e2e.tsx | Adds screenshot test for variant="tag" + size="small" states (light/dark). |
| e2e/screenshots/button-variant-tag-size-small-.png | New baseline screenshot (light theme). |
| e2e/screenshots/dark--button-variant-tag-size-small-.png | New baseline screenshot (dark theme). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frankensteinke
left a comment
There was a problem hiding this comment.
I think we'd probably want to add this new prop to the docs for Button and TagButton. Here's what Claude told me that would involve:
- docs/pages/components/Button.mdx — add a
sizeentry to theComponentPropsat line ~119 (noting it's only valid whenvariant="tag"), and optionally a new example under the Tag section (~line 81) showing<Button variant="tag" size="small">Small Tag</Button>.- docs/pages/components/TagButton.mdx — add a
sizeentry to theComponentPropsarray at line 30, and optionally an example alongside the one at line 15.Both use the shared
<ComponentProps>component, so it's just an object append like:{ name: 'size', type: ['default', 'small'], defaultValue: 'default', description: '...' }
Other than that, I just left some minor pieces of feedback. Thanks for contributing!
There was a problem hiding this comment.
Weird that these screenshot file names end with a hyphen, but I don't think it's related to anything in this PR.
There was a problem hiding this comment.
Agreed. Maybe we open new issue?
Co-authored-by: Tevin Steinke <51126809+frankensteinke@users.noreply.github.com>
Co-authored-by: Tevin Steinke <51126809+frankensteinke@users.noreply.github.com>
frankensteinke
left a comment
There was a problem hiding this comment.
Looks good! Just left some nits about other test .getByText calls that could be changed to .getByRole.
| setActive(component.getByText('Active')); | ||
| await component.getByText('Focus').focus(); |
There was a problem hiding this comment.
Nit: there are some other .getByText calls that could be updated to .getByRole.
| await component.getByText('Hover').hover(); | ||
| setActive(component.getByText('Active')); | ||
| await component.getByText('Focus').focus(); |
There was a problem hiding this comment.
Nit: more .getByText calls that could be updated to .getByRole.
Closes #1957
Summary
sizeprop ('default' | 'small') toButton(whenvariant="tag") andTagButtonTagSizetype from theTagcomponentButtonPropsinto discriminated union types sosizeis only valid whenvariant="tag"(type error otherwise)ButtonandTagButtonsizeprop in bothButton.mdxandTagButton.mdx:user-invalidto:invalidinselect.css, which caused required selects to show error styling before user interactionTextEllipsis[hideTooltip][maxLines=2]test to actually hover and assert no tooltip appearsTextEllipsis[hideTooltip]tests (component has no dark mode styling)Test plan
Buttonwithvariant="tag"andsize="small"renders withTag--smallclassButtonwith non-tag variant andsizeprop produces a TypeScript errorTagButtonwithsize="small"passes the size through to the underlyingButtonSelectfields show no error border before user interactionSelectfields show error border after user interacts and leaves the field emptyTextEllipsis[hideTooltip]tests hover and verify no tooltip is shown