fix(fuselage): align Button with the design system - #2151
Draft
josephjpearce wants to merge 13 commits into
Draft
Conversation
The button's rendered inset is the sum of its border and padding. The size mixin subtracted a hard-coded 2px on the assumption of a 2px border, but `--rcx-button-border-width` is 1px, so every button sat 1px tighter than specified — horizontally and vertically. The mixin now takes the *total* inset and subtracts the actual border width, and the per-size values are taken from the design system: - default 14px inline / 10px block, medium 14px / 8px, small 8px / 6px - a leading icon tightens the inset on the icon side (12/14, 12/14, 8/10) - the 40px and 48px sizes pair with a 20px icon; smaller sizes stay at 16px - `is="a"` + `external` gets a `new-window` leading icon unless one is supplied Visual-regression snapshots are generated on Linux and will need regenerating in CI; they are deliberately not updated here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ound `getPalette` mapped `button-background-secondary-danger-disabled` to `button.backgroundDangerDisabled` (#FFC1C9 — the filled danger disabled background) rather than `button.backgroundSecondaryDangerDisabled` (#EBECEF). Every sibling entry in the same list uses `backgroundSecondaryDanger*`, and the SCSS fallback chain already expected #EBECEF, so the `:root` custom property was the only thing overriding it. A disabled secondary danger button therefore rendered as a pink filled button instead of a greyed-out neutral one. Confirmed against the design system, where the variant uses Button/Background/Secondary danger/disabled with Button/Font/on-secondary-danger-disabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5668c6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Joseph RC seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
The previous commit expressed the per-size insets as raw pixel literals, which regressed the token-first rule. They are now composed from `lengths.padding()` and the border-width length, so no pixel value is written by hand. Two of the values the design system specifies — the 14px inline inset and the 22px leading-icon inset — are not directly expressible on the 4px scale (`lengths.padding()` accepts 1, 2, or a multiple of 4). They are built as `padding(16) - padding(2)` and `padding(24) - padding(2)` and flagged in the PR description; the cleaner fix is for the design to land on a scale value. Rendered output is unchanged — verified identical insets at every size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
josephjpearce
force-pushed
the
feat/button-figma-parity
branch
from
August 10, 2026 21:48
35acfd1 to
10c2e3e
Compare
Adds a story comparing the same icon + label pairing across the four rectangular sizes, with the icon canvas outlined and the 4px gap tinted so both are observable rather than inferred. Doc text states the intended rule: the canvas scales with the label's line-height, the gap does not, and the icon-side inset is one 4px step tighter than the label side. `mini` and `tiny` are left out of Storybook entirely, both here and in the size control. They type-check as `size` values but have no rectangular styles, so they silently fall back to the default box with a mismatched 16px canvas; offering them would present a broken option as a working choice. Their measurements are recorded in the PR's spacing audit instead, which is where the case for fixing or removing them belongs. `IconButton` is untouched — tiny and mini are square by nature there and work correctly. Also corrects two argTypes: `size` did not document the unset default, and `square` was described as replacing a "pill shape" when buttons are 4px-radius rectangles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
josephjpearce
force-pushed
the
feat/button-figma-parity
branch
from
August 11, 2026 15:21
10c2e3e to
53f047c
Compare
juliajforesti
changed the title
Button with the design system
juliajforesti
requested changes
Aug 19, 2026
Contributor
There was a problem hiding this comment.
Code comments should not be that verbose - I suggested changes removing references to previous code, PR description, and narrative justification, keeping only the non-obvious info needed to understand the following logic. Comments should not be a history tracker - PRs description are.
All relevant and necessary comments (for both dev and design team) can be added to the storybook description
Contributor
There was a problem hiding this comment.
No need for this patch changeset. We can rely on the minor changeset message here since the PR only has a few changes, that are already covered by the minor description.
Please delete this file
Comment on lines
+54
to
+56
| // `mini` and `tiny` are valid `size` values but have no rectangular | ||
| // styles, so they silently fall back to the default box. They are left | ||
| // out of Storybook entirely rather than presented as a working choice. |
Contributor
There was a problem hiding this comment.
no need for this comment.
If you need to clarify why those variations are not stated in this story, you can add to the story description so it's visible in the UI, not just in the code.
Suggested change
| // `mini` and `tiny` are valid `size` values but have no rectangular | |
| // styles, so they silently fall back to the default box. They are left | |
| // out of Storybook entirely rather than presented as a working choice. |
Comment on lines
+368
to
+436
| <table style={{ borderCollapse: 'collapse' }}> | ||
| <thead> | ||
| <tr> | ||
| {['Size', 'Icon + label', 'Label only', 'Icon only'].map((h) => ( | ||
| <th | ||
| key={h} | ||
| style={{ | ||
| padding: '8px 16px', | ||
| textAlign: 'left', | ||
| font: '700 12px/16px Inter, sans-serif', | ||
| color: '#6C727A', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| {h} | ||
| </th> | ||
| ))} | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {SIZE_ROWS.map(({ label, size }) => ( | ||
| <tr key={label}> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| font: '700 12px/16px Inter, sans-serif', | ||
| color: '#6C727A', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| {label} | ||
| </td> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| <Button variant='primary' size={size} icon='baloon-text'> | ||
| Button | ||
| </Button> | ||
| </td> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| <Button variant='primary' size={size}> | ||
| Button | ||
| </Button> | ||
| </td> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| <Button | ||
| variant='primary' | ||
| size={size} | ||
| square | ||
| icon='baloon-text' | ||
| /> | ||
| </td> | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> |
Contributor
There was a problem hiding this comment.
use PropsVariationSection to build the table variations
| padding-block: calc( | ||
| (#{lengths.size($height)} - #{$line-height}) / 2 - #{$border-width} | ||
| ); | ||
| padding-inline: calc(#{$inline-inset} - #{$border-width}); |
Contributor
There was a problem hiding this comment.
no need for #{} here too
| } | ||
|
|
||
| @mixin with-leading-icon-inset($start, $end) { | ||
| padding-inline: calc(#{$start} - #{$border-width}) |
Contributor
There was a problem hiding this comment.
no need for #{} here too
| (large && 'large') || | ||
| undefined; | ||
|
|
||
| const effectiveIcon = resolveIcon(icon, is, external); |
Contributor
There was a problem hiding this comment.
I understand the motivation and this results in a more concise and drift proof devExp - but i'm wondering whether we shouldn't have an opt-out (e.g. allow icon={false}) ?
maybe there are cases where there's no space for icon + text, idk...
Author
There was a problem hiding this comment.
I think that makes sense to allow that.
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.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
3 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.
fix(fuselage): align Button with the design system
Closes DSN-182.
Audits
Buttonagainst the design system Component section, fixes the deviations, and adds a story documenting how a leading icon pairs with a label across sizes.Four commits, reviewable independently:
secondary-dangerbackground — a mis-mapped palette entryIcon and labelstory — plus twoargTypesdoc correctionsMethod
Every number below was read off the rendered DOM (
getComputedStyle+ bounding boxes), not from source. The inset figures are total edge-to-content distances — border plus padding — because that is what is comparable to a Figma frame with no stroke.Colour expectations come from
@rocket.chat/fuselage-tokens, which is the design-token export of the Figma library; the correspondence was spot-checked directly in Figma on primary (default/hover/press/keyfocus), secondary (default) and secondary-danger (default/disabled).1. The underlying spacing bug
A button's rendered inset is the sum of its border and its padding. The size mixin subtracted a hard-coded
2pxon the assumption of a 2px border:padding-inline: calc(lengths.padding($padding-x) - 2px);--rcx-button-border-widthis1px, so every button sat 1px tight on every edge — horizontally and vertically, the vertical shortfall pushing the label 1px off-centre inside the fixed height. The mixin now takes the total inset and subtracts the actual border width, so the value in the stylesheet is the value you measure.Spacing audit — every supported size
Insets are
start / end / block. "Canvas" is the icon box, not the glyph.minitinysmallmediumlargeBefore → after
is="a" externalleading iconnew-windownew-windowIcon-only surrounds now match exactly: 6/6/6/6 at 28, 8/8/8/8 at 32, 10/10/10/10 at 40. The 2px offset came from
marginInlineEnd={4}being applied even with no label to separate from; it is now applied only when a label is present. Being a constant 2px, it read worst on the smallest button.Icon/label vertical alignment
Icondefaults tovertical-align: text-bottom, which drags the shared baseline down and pushes the label below centre. Scaling the default canvas to 20px made this visible; it was latent at the smaller sizes. Label offset from centre, positive = below:mainThe residual −0.5 is present on text-only buttons on
maintoo — font ascent/descent asymmetry within the line box, left alone.Token provenance — one deviation
All spacing now derives from
lengths.padding()andlengths.border-width(); no pixel literal is written by hand. But two design values are not expressible on the 4px scale —lengths.padding()accepts 1, 2, or a multiple of 4:padding(16) - padding(2)padding(24) - padding(2)10px (
padding(8) + padding(2)) is in the same category. These compose from scale steps rather than being literals, but they are off-scale values. The cleaner fix is a design decision — either move the inline inset to 12 or 16, or add the step to the scale. Flagging rather than silently absorbing it.Block insets need no such treatment: they derive from height and line-height, both already on the scale.
2. Colour parity — state × variant
loadingis included: it setsdisabled, so its colours are identical to disabled in every case (verified, not assumed). Icon colour inherits the label colour in all 48 cells. Border colour equals background in every non-focus state.primarysecondarydangersecondary-dangersuccesswarningsecondary-warningsecondary-successNo colour mismatches remain. Every variant/state with a design token matches the rendered value exactly.
¹ Value correct, but the token key is
onSecondaryDangerDisabledwhere every sibling isfontOn*. The emitted CSS property is right (--rcx-color-button-font-on-secondary-danger-disabled, matching Figma'sButton/Font/on-secondary-danger-disabled). Cosmetic — but it is what made the mis-mapping in §3 easy to miss.²
warning,secondary-warningandsecondary-successare driven by primitives, not semantic button tokens, and have no Figma counterpart at all.success(filled) does have tokens. Out of scope to invent values; raised as a design-system gap.Focus ring
Every variant: 1px
#2F343Dborder (stroke-extra-dark) plus a 2px outer ring — structurally matching Figma, which draws a 2pxStroke/extra-light-highlightring outside aStroke/extra-darkborder.focus-shadow-colortoken, so it falls back--rcx-color-shadow-highlight(used in code) and--rcx-color-stroke-extra-light-highlight(Figma's name) both resolve to#D1EBFE— naming difference only, no visual difference. Worth reconciling in the token export.Token names, code vs Figma
button-background-{v}-focusButton/Background/{V}/keyfocus--rcx-color-shadow-highlightStroke/extra-light-highlightbutton-font-on-secondary-danger-disabledButton/Font/on-secondary-danger-disabled3. Disabled
secondary-dangerbackgroundgetPalettemapped it to the filled danger token:{ name: 'button-background-secondary-danger-disabled', - color: button.backgroundDangerDisabled, // #FFC1C9 + color: button.backgroundSecondaryDangerDisabled, // #EBECEF },Every sibling in that list uses
backgroundSecondaryDanger*, and the SCSS fallback chain already resolved to#EBECEF— the:rootcustom property was the only thing overriding it. A disabled secondary danger button rendered as a pink filled button instead of a greyed-out neutral one.Confirmed in Figma: Secondary / Disabled / Danger Action = True uses
Button/Background/Secondary danger/disabledwithButton/Font/on-secondary-danger-disabled.4.
Icon and labelstoryNew story under Inputs / Button, covering the four rectangular sizes —
small,medium, default (unset),large— side by side with:--with-icon, so it does not draw where no label follows)It breaks visibly if canvas or gap drift.
miniandtinyare deliberately absent from Storybook entirely — both the control and this story. They type-check assizevalues but have no rectangular styles, and presenting a broken option as a working choice is worse than leaving it out of the UI. Their measurements are retained in the spacing table above, which is where the case for fixing or removing them belongs.Also corrects two
argTypes:sizenow documents the unset default (40px) and offerssmall/medium/largeonly.squarewas described as replacing a "pill shape"; buttons are 4px-radius rectangles.Before / after screenshots
Attached in the first comment. Every visual claim above is also backed by measured numbers, so the review stands without them.
before/label-undefined.pngafter/label-undefined.pngbefore/label-medium.pngafter/label-medium.pngbefore/label-small.pngafter/label-small.pngbefore/icon-undefined.pngafter/icon-undefined.pngbefore/icon-medium.pngafter/icon-medium.pngbefore/icon-small.pngafter/icon-small.pngbefore/square-undefined.pngafter/square-undefined.pngbefore/square-medium.pngafter/square-medium.pngbefore/square-small.pngafter/square-small.pngbefore/external.pngafter/external.pngsecondary-dangerdisabledbefore/sd-disabled.pngafter/sd-disabled.pngsecondarydisabled (reference, unchanged)before/s-disabled.pngafter/s-disabled.pngdangerdisabled (reference, unchanged)before/d-disabled.pngafter/d-disabled.pngmatrix-before.pngmatrix-after.pngIntentionally out of scope
mini/tinyrectangular styles. They accept asizebut only have square definitions, so they inherit the default box while taking a 16px canvas — internally inconsistent withdefault. Figma has no spec for them, so there is nothing to implement against. Documented in the spacing table rather than guessed at, and kept out of Storybook so nothing advertises them as working. This is the one acceptance criterion in the ticket not satisfied — "no size silently inherits values from another" still fails for these two. It needs a decision: give them real rectangular styles, or drop them fromButtonProps['size']as a breaking change.IconButtonis unaffected —tiny/miniare square by nature there, the styles exist, and they are used inSidebarBanner,SidebarV2andToastBar.large(48px). No Figma counterpart. Its inline inset is set to the token value (24px), moving the rendered inset 23px → 24px.size="large" squarerenders 40px, not 48px — pre-existing, left alone.warning/successfamilies. No Figma counterpart and no semantic tokens; see §2 note ².ubuntu-latest; regenerating on macOS would rewrite every file with font-rasterisation noise. Allinputs-button--*andinputs-buttongroup--*snapshots need regenerating in CI, plus any story embedding a default-size Button (toastbar, codesnippet, throbber, card, callout), plus a new snapshot for the story added here.Test plan
yarn tsc -p tsconfig.build.json --noEmit— cleanyarn eslint src/components/Button— cleanyarn stylelint src/components/Button/Button.styles.scss— cleanyarn jest src/components/Button src/components/PaletteStyleTag— 9 passed