fix(Button): treat a string label like a Text child - #3053
Merged
Conversation
Lisa18289
added
update-screenshots
github-actions
Bot
removed
run-visual-tests
Contributor
Coverage Report for ./packages/components/
File CoverageNo changed files found. |
Contributor
🚀 Preview DeploymentPreview environments are ready:
Images:
|
Contributor
❌ Visual Regression Tests FailedAt least one shard did not pass. If snapshots differ from the committed baselines, download the visual-diffs-* artifacts from this run to inspect the actual/diff images (a shard that failed before comparison, e.g. during install or browser setup, leaves none). If the differences are intentional, update the baselines by adding the |
Lisa18289
changed the title
Lisa18289
added
update-screenshots
github-actions
Bot
removed
run-visual-tests
Lisa18289
force-pushed
the
claude/button-text-icon-link-spacing-b2b459
branch
from
September 3, 2026 11:12
e300f2e to
656806b
Compare
Link wrapped all Button children in a single `Text`, collapsing icon and label into one flex item. The button's `column-gap` then had nothing to apply to, and the icon lost its `flow--button--icon` class, so the `:has(.text) .icon` margin didn't apply either — icon and label ended up glued together. Wrap only plain string children, the same condition `Button` itself uses. This also restores the icon-only padding for an icon-only Button in a Link, which the always-present `.text` had suppressed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Lisa18289 <84317589+Lisa18289@users.noreply.github.com>
Drop the WithButtonAndIcon story — the visual test already covers it — and trim the comment on the Text wrapper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A raw string child rendered no element, so `:has(.text)` never matched it:
`<Button><Icon />Label</Button>` got the icon-only padding (8px instead of
16px), and a plain pending button hid its label. Which branch a button took
depended on how the label was written — `Label` and `{x} Label` behaved
differently.
The button now carries the marker itself, so a string is detected wherever
it sits, including through the fragment `Link` passes down. That drops the
`Text` wrapper from both `Button` and `Link`; `.content` keeps the two
properties it contributed.
Also removes a dead selector: `&:where(:has(.text) .icon)` compiles to a
compound selector that can never match.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Lisa18289 <84317589+Lisa18289@users.noreply.github.com>
Lisa18289
force-pushed
the
claude/button-text-icon-link-spacing-b2b459
branch
from
September 3, 2026 11:15
656806b to
a22e769
Compare
The plain variant kept its content visible while pending, succeeded or failed, so the state icon was drawn on top of the label. The exception only ever fired for a single string child, so no scenario showed it until string labels started matching. Plain now behaves like every other variant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Lisa18289
added
update-screenshots
github-actions
Bot
removed
the
update-screenshots
Co-authored-by: Lisa18289 <84317589+Lisa18289@users.noreply.github.com>
Lisa18289
marked this pull request as ready for review
September 4, 2026 04:28
mfal
reviewed
Sep 4, 2026
`<Button><Icon />{42}</Button>` matched no text marker and got the
icon-only padding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mfal
enabled auto-merge (squash)
September 4, 2026 06:17
mfal
approved these changes
Sep 4, 2026
This was referenced Sep 4, 2026
mfal
added a commit
that referenced
this pull request
Sep 4, 2026
#3053 landed `containsTextChild` with its own `isTextNode` predicate and its own fragment recursion, which is the same classification `joinTextChildren` does here. Two traversals, one piece of knowledge. `containsTextChild` now asks `extractTextFromChildren` and compares against `undefined`, so `isTextNode` goes away. One behavior change follows from the shared reading: `extractTextFromChildren` trims, so children that are only whitespace no longer count as text. That is the right answer for the question `Button` and `Link` ask — a blank string is not a label, and the control should keep its icon-only layout. Pinned by a test. The boolean answer no longer short-circuits at the first text child. Irrelevant at the child counts these components see. Also drops `Button` from `extractTextFromFirstChild`'s doc comment: #3053 replaced its call with `containsTextChild`, leaving `Markdown`, `Initials` and `Truncate` as the callers that depend on the strict reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Lisa18289
added a commit
that referenced
this pull request
Sep 4, 2026
#3053 landed the same predicate in `remote.ts` for the button's text marker. Export it and drop the local copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Lisa18289
added a commit
that referenced
this pull request
Sep 4, 2026
#3053 landed the same predicate in `remote.ts`. Applied per child it answers exactly this question, and it looks through fragments, so `<>{"Text"}</>` is wrapped and truncated too — the local helper returned false for that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
Buttonwith an icon and a label lost the spacing between them inside aLink. Digging into that surfaced the same root cause one level down, so this fixes both.Linkwrapped allButtonchildren in a singleText, collapsing icon and label into one flex item — the button'scolumn-gaphad nothing to apply to.Buttononly wrapped children inTextwhen they were a lone string, and:has(.text)was the only signal that a button contains text. So a raw string next to an icon matched nothing:<Button><Icon />Label</Button>got the icon-only padding (8px instead of 16px), and a plain pending button hid its label. Which branch a button took depended on how the label happened to be written —Labeland{x} Labelbehaved differently.The button now carries the marker itself, so a string is detected wherever it sits, including through the fragment
Linkpasses down. That drops theTextwrapper from both components;.contentkeeps the two properties it contributed (text-align,text-wrap-style).Also removes a dead selector:
&:where(:has(.text) .icon)compiles to.flow--button:where(:has(.flow--button--text) .flow--button--icon)— a compound selector that can never match.Baselines: three scenarios change on purpose — the icon+string padding, the plain pending label, and sub-pixel text rendering where a label is no longer inside a span. The darwin baselines are updated; the linux ones come from the
update-screenshotslabel.🤖 Generated with Claude Code