Skip to content

dogfooding fixes: Button size uses min-height so theme padding overrides compose#3472

Open
josephfarina wants to merge 1 commit into
mainfrom
navi/fix/button-min-height
Open

dogfooding fixes: Button size uses min-height so theme padding overrides compose#3472
josephfarina wants to merge 1 commit into
mainfrom
navi/fix/button-min-height

Conversation

@josephfarina

Copy link
Copy Markdown
Contributor

What this fixes

Increasing a Button's height via a theme paddingBlock override silently did nothing. Button size styles hard-coded a fixed height per size, and under box-sizing: border-box that fixed height absorbed any padding added in the theme — no error, no visual change. Fixes #3379.

Root cause

The base style sets paddingBlock: 8px with line-height ≈ 20px, so the natural content height is 36px. Each size then pinned a fixed height (sm 28 / md 32 / lg 36), clamping the content below natural. A theme override like:

button: { 'size:lg': { paddingBlock: '20px' } }

emits .astryx-button.lg { padding-block: 20px } — but the button stays 36px tall because the fixed height is still in effect and border-box makes the extra padding eat into those 36px. Result: no visible change.

The trap, measured (real Chrome render)

Fixed height: default sm/md/lg render 28/32/36. A naive heightmin-height swap is not safe — it makes every sm/md button jump to the 36px natural content height (a system-wide regression), and it collapses icon-only buttons (which use aspect-ratio: 1/1 with zero padding, so height defines the square):

Button height model — the problem and why a naive swap regresses

The fix

Replace the fixed height with min-height per size and move a per-size padding-block (sm --spacing-1 / md --spacing-1-5 / lg --spacing-2) into the size styles, removing the base padding-block. This keeps default heights pixel-identical, preserves icon-only squares, and lets a theme padding override grow the button.

Verified in real Chrome — every check passes: default label heights stay exactly 28/32/36, icon-only stays square (28×28 / 32×32 / 36×36), and a paddingBlock override grows the button (lg + pad20 → 60px):

Button fix — default heights preserved, icon-only square, padding grows the button

Why not just height: auto on override (the issue's workaround)?

That requires every theme author to know the non-obvious height: 'auto' trick. Making min-height the default means the natural theming lever (paddingBlock) composes as expected, with no workaround.

Verification

  • Compiled CSS (scripts/build-css.test.mjs): new regression test asserts the Button size classes emit min-height:var(--size-element-*) and per-size padding-block, and that the size rule carrying min-height does not also pin a fixed height. (This is the source-of-truth layer — StyleX atomic classes aren't visible to jsdom.)
  • Component tests: all 30 Button unit tests pass.
  • Storybook: new "Padding grows the button (Button hard-codes a fixed height per size, silently overriding theme paddingBlock overrides (theming a taller button requires non-obvious height: auto) #3379)" story shows a default lg button beside one with a paddingBlock override.
  • Icon-only padding-block:0 + aspect-ratio confirmed intact in the compiled CSS (icon-only style is applied after the size style, so it still wins).
  • TypeScript typecheck and ESLint pass.

Button size styles set a fixed `height` per size, which under
`box-sizing: border-box` absorbed any theme `paddingBlock` override — so
theming a taller button silently did nothing. Replace the fixed `height`
with `min-height` and move a per-size `paddingBlock` (sm/md/lg) into the
size styles (removing the base `paddingBlock`). Default heights (28/32/36)
and icon-only square sizing are unchanged, but a padding override now grows
the button.

Fixes #3379
@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview, Comment Jul 2, 2026 9:54pm

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 2, 2026
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

No new or modified components detected.

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.6KB 0B

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Button hard-codes a fixed height per size, silently overriding theme paddingBlock overrides (theming a taller button requires non-obvious height: auto)

1 participant