fix(button): centre icons natively, fix hover and disabled states#7402
Open
talissoncosta wants to merge 16 commits intomainfrom
Open
fix(button): centre icons natively, fix hover and disabled states#7402talissoncosta wants to merge 16 commits intomainfrom
talissoncosta wants to merge 16 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
Bot
added
the
front-end
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
Contributor
Docker builds report
|
12 tasks
Contributor
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Contributor
Visual Regression16 screenshots compared. See report for details. |
Zaimwa9
previously approved these changes
May 4, 2026
talissoncosta
requested review from
Zaimwa9 and
khvn26
and removed request for
a team
Contributor
|
@talissoncosta can you rebase main please |
talissoncosta
force-pushed
the
fix/button-icon-spacing
branch
from
d14de72 to
22626b0
Compare
talissoncosta
added a commit
that referenced
this pull request
May 6, 2026
Port from #7402 (commit 20ea19b) — no production caller uses theme='project'. The avatar tile rendering uses className='btn-project' directly, bypassing the typed theme map. Drop the dead entry from themeClassNames + remove the matching example from the Variants story. The .btn-project SCSS class itself stays — 6 production sites (ProjectManageWidget, OrganisationsPage, ImportPage) still use it via raw className. They'll be addressed when the Tile / BareButton primitive lands (see BUTTON_LINK_DESIGN_PLAN.md). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The icon-spacing issue (originally PR #7402) had multiple root causes: the iconLeft/iconRight prop API created a separate render path from the children-based pattern that production code mostly uses, and patching one path's spacing repeatedly caused regressions in the other. Removing the prop API entirely is the cleaner fix — there's now one render path for icons, and spacing is handled by the button's flex+gap wrapper. Changes: - Remove iconLeft/iconRight/iconLeftColour/iconRightColour/iconSize props (and the local iconColours map) from Button.tsx. - Single content wrapper for both <button> and <a> render paths: <span className='d-flex h-100 align-items-center justify-content-center gap-2'> {children} </span> - Migrate 7 production callers to compose icons as children: EditIdentity, JSONReference, PipelineStageActions, GoogleButton, ModalConfirm, FlagEnvironmentsPage, HomePage. - Pre-existing type bug fixed in ModalConfirm: `iconRight='fas fa-trash'` was an invalid IconName, replaced with `<Icon name='trash-2' />`. - Add components/icons/index.ts barrel export so callers can use `import { Icon } from 'components/icons'` instead of the path-based default import. - Update Button stories: WithIcons composes icons as children; IconOnly story added covering theme='icon' and btn-with-icon variants. Icon colour now inherits from the button's text colour by default (currentColor), so no hardcoded fills are needed in callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit always wrapped Button children in a flex+gap span, which regressed icon-only callers (`btn-with-icon` rows like the identities trash button) and presentational containers (`btn-project` project tiles in ProjectManageWidget). For single children the wrapper is unnecessary — render children directly so callers' own layout isn't disturbed. The wrapper still applies when there are multiple children, which is the case the spacing fix targets (icon + label). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…utton The prior commit replaced an invalid `iconRight='fas fa-trash'` (a Font Awesome class string passed where the prop expected a project IconName, which silently rendered nothing in production) with `<Icon name='trash-2' />`. That introduced an oversized 24px icon that production never displayed. The right fix is to drop the icon entirely so the modal matches prod's behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
No production caller referenced `theme='project'` — every project tile uses the raw `className='btn-project'` SCSS class instead, going around the typed theme map. The entry was only kept alive by Button.stories' Variants story. Removing it tightens the typed theme surface to themes that actually have callers. The `btn-project` SCSS class itself stays in _buttons.scss; the 6 production sites that use it (ProjectManageWidget, OrganisationsPage, ImportPage) keep working unchanged. A future PR will extract a Tile / BareButton primitive to replace those className-driven tiles (and fix the two `<a><button>` invalid-nesting sites along the way), at which point `btn-project` itself can be retired. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The .btn-icon resting/hover states hardcoded \$text-muted and \$body-color SCSS variables that didn't adapt between light and dark — leaving icons illegible against dark backgrounds (visible in the new IconOnly story). Replace with --color-icon-secondary (resting) and --color-icon-default (hover). Both tokens auto-resolve via CSS variables: slate-500/600 in light, slate-300/0 in dark — proper contrast in both modes. Drops the now-redundant path-fill overrides from the .dark block since the CSS vars handle the mode switch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The base \`.btn\` rule sets \`color: white\` as the default, which the
\`btn-with-icon\` resting state silently inherits — so icons render white
on the class's light-grey \`\$basic-alpha-8\` background in light mode
(invisible) and white on the dark variant (visible by accident).
Production callers compensate ad-hoc: some pass \`text-body\` className,
others hardcode \`fill='#656D7B'\` on the Icon. Either escape works, but
the class itself is fragile — drop the explicit fill on a caller and it
falls into the white-on-grey case.
Set \`svg path { fill: var(--color-icon-secondary) }\` on the resting
state so the visual treatment is self-contained. Hover/focus rules keep
their existing purple/body-color overrides — the resting fix is purely
additive. Token auto-adapts: slate-500 in light, slate-300 in dark.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the IconOnly story — it duplicated the `theme='icon'` example already in Variants and documented the `className='btn-with-icon'` hack as a Button feature, which it isn't (it's a className override that will be replaced by a future IconButton primitive). Also trim the related hint from the Variants description so it doesn't push readers toward `theme='icon'` for table actions. Add the missing `xxSmall` variant to the Sizes story so visual coverage matches `sizeClassNames`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the project's `.btn` from Bootstrap-default `inline-block` to `inline-flex` with `align-items: center; justify-content: center; gap: 0.5rem`. This lets icon+label compositions space themselves via flex gap natively — no wrapper span needed — and centres icon-only buttons without the `mr/ml` margin trick that was the original spacing bug. `Button.tsx`'s conditional flex-wrapper goes away, and `themeClassNames` is normalised so `<a>` and `<button>` paths share className computation (links also pick up `.btn` consistently). Two fallout fixes from that switch: - Pin `height: $btn-line-height` on `.btn` so default-size buttons keep their 44px box. Without it, inline-flex collapses to content height (was implicitly 44px from Bootstrap's line-height-as-height). Also drop the legacy `.btn:hover` / `.btn:active` rules that forced `$btn-hover-bg` (purple) onto every variant — they were project-picker leftovers that leaked purple onto Danger / Tertiary / etc. when their variant-specific hover happened to share specificity. - Drop the matching dark-mode blanket `.dark .btn:hover` / `.dark .btn:active` rules that re-introduced the same purple at higher specificity (0,3,0 — beats variant `.btn-danger:hover` at 0,2,0). Variants and Bootstrap-generated `.btn-primary:hover` now own each variant's hover state cleanly in both themes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`.btn-project` is the project picker tile — a 76px-tall surface whose letter+title layout is owned by the inner `<Row>`, not by the outer button. Inheriting `.btn`'s new `inline-flex / align-items: center / gap: 0.5rem` collided with the tile's internals and produced inconsistent letter sizing across project tiles. Set `display: inline-block` on `.btn-project` so the tile keeps its old block-level layout. The flex-related properties from `.btn` become inert once display is no longer flex. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…16px
The "Invite additional member" button wrapped its icon+label in a
`<Row>` with `pl-2 icon` on the icon span — that adds padding INSIDE
the icon span (left of the icon), not between icon and text, so the
icon and label touched each other. Drop the wrapper and pass the icon
and text directly as Button children; `.btn`'s `gap: 0.5rem` now
spaces them.
Also swap the IonIcon `add` glyph for the project's `<Icon name='plus'>`
to drop the third-party icon library on this consumer. Default plus
icon size (21×20) is too heavy on a `size='small'` outline button —
set `width={16}` to match the most common icon-in-small-button norm
across the codebase (UserAction trash, PanelSearch refresh, etc.).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ViewDocs was working around `.btn`'s old inline-block layout with three hacks: `marginTop: -5` on the icon, `lineHeight: 24px` on a wrapper span, and a leading space inside the label string for spacing. With `.btn` now `inline-flex; align-items: center; gap: 0.5rem`, all three become unnecessary — the icon centres natively and gap handles the icon→label space. The hardcoded `fill='#6837fc'` stays for now (a dark-mode concern that belongs with the broader semantic-token migration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For files this PR has already touched, replace static hex fills on
`<Icon>` with their semantic token equivalents — or drop the fill
prop entirely where the parent already sets the right colour and the
icon defaults to `currentColor`:
- ViewDocs: `#6837fc` → drop the prop. The icon now inherits via
`currentColor` from `.btn-link`'s `$primary` colour.
- InviteUsers invite-row trash: `#656D7B` → drop the prop.
`.btn-with-icon` SCSS already sets `svg path { fill:
var(--color-icon-secondary) }`, the explicit prop was redundant.
The static hexes were invisible-or-wrong in dark mode (one of the
findings in the design-system audit, #6606). The broader migration
of the remaining ~85 hex-fill icons stays as a follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the `.btn` switch to inline-flex, `theme='text'` buttons (which
render as `.btn-link`) didn't behave as inline links anymore — they
inherited the new 44px height pin and rendered as 44px boxes inline
with surrounding text, throwing off any header that placed an Edit
affordance next to a label (e.g. the alias row in the identity page).
Make `.btn-link` properly inline-flow:
- `display: inline-flex; align-items: center; gap: 0.25rem; vertical-
align: baseline; height: auto` — the button hugs its content,
centres icon+label internally, and aligns to the surrounding text
baseline. Consumers no longer need `d-flex align-items-center` to
centre a `theme='text'` button's children — the rule does that now.
- Selector includes `button.btn-link` so `height: auto` beats
`button.btn { height: 44px }`'s type-selector specificity for
`<button>` elements (it was only winning on `<a>` before).
- Drop the now-redundant `d-flex align-items-center` on the alias
Edit button in `EditIdentity.tsx`.
Also fix the identity-page header row that was using `align-items:
end` on the parent `<h6>` — bottom-aligning flex items by box edges
puts each child's text at a different vertical position. Switch to
`align-items: baseline` so the text baselines of all flex children
(label, GhostInput, Edit button) line up properly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
talissoncosta
force-pushed
the
fix/button-icon-spacing
branch
from
bae8de9 to
1a0db6a
Compare
…ssing dark-mode :active for success and outline Two related cleanups in the same family of cascade bugs we fixed earlier in the PR. `.btn-success` was using `\$success` / `\$success400` / `\$success600` in an order that didn't match the project's intended progression (which exists in `_variables.scss` as `\$btn-success-*` but wasn't being consumed). Switch the SCSS to consume those variables so resting → hover → active progresses lighter → mid → darker, in line with how every other variant feels — applied in both light and dark mode. In dark mode, both `.dark .btn.btn-success:hover` and `.dark .btn--outline:hover` had specificity (0,4,0) — higher than the light-mode `:active` rules at (0,3,0). Clicking-and-holding fired hover, focus and active simultaneously, and the dark-mode hover won by specificity, so the active state never visually distinguished from hover. Add explicit `:active` overrides at the same (0,4,0) specificity tier so active wins via source order — same family as the purple-bleed fix on `.btn:hover` we did earlier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bootstrap's `button-variant` mixin auto-generates `:disabled` rules with a contrast-computed colour. With the variant's lighter-bg disabled blend (`opacity: 0.65` on top of the green/red), Bootstrap picks black as the "best contrast", which clashes with the white text the variants use in every other state. Add explicit `color: white` / `color: \$text-icon-light` overrides on `.btn-danger:disabled` and `.btn-success:disabled` so the disabled state reads as "muted action button" (faded bg + white text), not "different-coloured button". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The story description was exposing `display: flex; gap: 0.5rem` to consumers. Storybook is a usage guide, not a reference for the underlying CSS — and pinning the description to today's implementation means the doc rots when the layout strategy changes (e.g., Tailwind migration). Reword to focus on the public contract: icons go as children, spacing is Button's job. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
talissoncosta
changed the title
talissoncosta
changed the title
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.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Follow-up to a review comment on #7100 — icon-only buttons (
theme='icon') rendered with the icon visibly off-centre. The cause was margin-based spacing:iconLeftandiconRightprops appliedmr-2/ml-2directly to the icon. On icon-only variants (32×32 withjustify-content: center), the trailing margin pushed the icon a few pixels off-centre — flexbox re-centres the item itself, but the margin still shifts the content inside.Rather than patching the margin, this PR drops the icon-position props and lets
.btnhandle spacing natively.Button API
iconLeft/iconRightprops. Icons now compose as children alongside the label..btnlayoutinline-blocktoinline-flexwithalign-items: center; gap: 0.5rem— siblings space themselves, no wrapper span or margin tricks, icon-only buttons centre by construction.height: $btn-line-heightso default-size buttons keep their 44px box (inline-flex would otherwise collapse to content height)..btn:hover/.btn:activerules (and their.darkcounterparts) that forced a purple background on every variant — they were project-picker leftovers leaking onto Danger / Tertiary / Success in both modes.Variant adjustments
.btn-link(theme='text') is nowinline-flexwithvertical-align: baselineandheight: auto, so it flows inline with surrounding text instead of rendering as a 44px box..btn-project(project picker tiles) opts out of the new flex layout — it's a 76px tile surface, not a button-shaped affordance.Consumer cleanups
InviteUsers+icon: drop the broken<Row>+ padding wrapper, swap IonIcon for the project's<Icon name='plus'>at 16px (the small-button norm).ViewDocs: drop the manualmarginTop: -5/lineHeight: 24pxalignment hacks now that.btndoes centring natively.fillvalues on icons in the touched files → semantic tokens (or dropped where the parent'scoloralready drives the icon viacurrentColor).Storybook
xxSmallvariant to the Sizes story.theme='icon'for table actions (a futureIconButtonprimitive will own that pattern).How did you test this code?
Components/Button→ walked through Variants / Sizes / Disabled / WithIcons.Editbutton baseline-aligned with the surrounding text.+icon spaced cleanly from the label.