refactor: simplify Button CSS by using min-heights

[pomfrida]

Summary

• Remove <TypographyNext> wrapper span from Button — children are now direct flex children
• Replace padding-derived sizing with explicit height values from Figma
• Eliminate all density overrides outside @layer (line-height overrides, asymmetric padding, icon-only padding calculations)
• Move comfortable density rules inside @layer eds-components

Result: 303 → 179 lines CSS (-41%), 0 calc(), 0 custom properties, 0 line-height overrides

Context

See #4513 for detailed analysis of the density token mismatch and trade-offs.

Test plan

• All 63 Button tests pass (snapshot updated)
• Storybook: all sizes, variants, icon-only, round — spacious and comfortable
• Measured button dimensions in browser — all match Figma exactly

	Spacious	Comfortable
sm	24px	20px
md	36px	24px
lg	44px	36px
icon-sm	24×24	20×20
icon-md	36×36	24×24
icon-lg	44×44	36×36

[pomfrida]

@claude review

[claude]

Claude finished @pomfrida's task in 1m 9s —— View job

---

Code Review

Summary

This is an excellent refactor that significantly simplifies the Button component. The changes are well-executed and demonstrate strong understanding of both CSS and React patterns. The 41% reduction in CSS complexity (303 → 179 lines) is achieved through a cleaner, more maintainable approach.

✅ Strengths

1. Simplified Component Structure (Button.tsx:50)

   • Removing the TypographyNext wrapper span makes children direct flex children
   • Cleaner DOM output (confirmed in snapshot)
   • Eliminates wrapper complexity while maintaining all functionality

2. Explicit Height Values (button.css:36-65)

   • Using explicit height values from Figma is more straightforward than padding-derived sizing
   • Clear TODO comment about future token replacement shows forward thinking
   • Comfortable density overrides are clean and minimal

3. Eliminated Complex Calculations (CSS diff)

   • Removed all calc() expressions for padding adjustments
   • Eliminated custom properties for density overrides
   • No more asymmetric padding or icon-only padding formulas
   • This makes the CSS far more maintainable

4. Icon-Only Simplification (button.css:70-73)

   • aspect-ratio: 1 + padding: 0 is elegant
   • Leverages flexbox for centering instead of complex padding calculations
   • Much easier to understand than the previous formula-based approach

5. Layer Organization (button.css:1-172)

   • Comfortable density rules properly moved inside @layer eds-components
   • Only the display override remains outside (with clear comment explaining why)
   • Good CSS architecture

6. Test Updates (Button.test.tsx)

   • Tests correctly updated to reflect new structure
   • Removed DOM queries for wrapper spans
   • All 63 tests passing confirms no functional regressions

🔍 Minor Observations

1. Typography Data Attributes Still Present (Button.tsx:40-44)

   • The component still sets data-font-family, data-font-size, data-line-height, etc.
   • These attributes suggest typography token usage but now there's no explicit typography component
   • Question: Are these still being consumed by CSS somewhere? If not, could they be removed for further simplification?
   • Files: packages/eds-core-react/src/components/next/Button/Button.tsx:40-44

2. Unused Variables (Button.tsx:30-31)

   • typographySize and selectableSpace are defined but used for the same mapping
   • The sizeToTypography constant (line 10) seems redundant with sizeToSelectableSpace
   • Could be simplified to just one variable
   • Files: packages/eds-core-react/src/components/next/Button/Button.tsx:30-31

3. Hardcoded Heights Comment (button.css:28-29)

   • The TODO is clear about replacing with --eds-selectable-height token
   • Good practice to document this technical debt
   • No action needed, but ensure this gets tracked in the backlog

✨ Code Quality

• EDS Conventions: ✅ Follows all conventions (vanilla CSS + BEM + tokens, forwardRef, named exports)
• Accessibility: ✅ All axe tests passing, no regressions
• Testing: ✅ Comprehensive test coverage maintained
• Readability: ✅ Significantly improved with removal of complex calculations
• Maintainability: ✅ Much easier to understand and modify

📊 Impact Assessment

Benefits:

• 41% CSS reduction (124 lines removed)
• Zero calc() expressions
• Simpler mental model (explicit heights vs. padding derivation)
• Easier debugging and modification
• Matches Figma exactly (validated in test plan)

Trade-offs:

• Hardcoded pixel values vs. token-derived (mitigated by TODO + clear path to tokens)
• Must update CSS when Figma heights change (acceptable trade-off for clarity)

🎯 Recommendation

LGTM - Ready to merge after addressing optional simplifications.

The refactor achieves its stated goals perfectly. The only suggestions are optional cleanups around the unused typography-related code. The core changes are solid and well-tested.

---

Optional follow-up improvements:

1. Consider removing unused typography data attributes if they're not consumed by CSS
2. Simplify variable names (typographySize / selectableSpace → single variable)

These are minor and don't block merging this excellent refactor.

---

[pomfrida]

Notes on CSS approach

Text centering (text shift)

Button text has an inherent ~0.5px vertical offset due to font metric asymmetry (ascender > descender). This is a property of the font — not a CSS bug.

Before (main): Padding-driven sizing with manual adjustments — asymmetric padding for comfortable density (padding-top: 5px / padding-bottom: 3px), line-height overrides per density/size, and calc() formulas to compensate for icon negative margins. Complex and fragile.

After: min-height controls button height. No padding-block — the span keeps its natural line-height and is centered by flexbox. The symmetric half-leading means flexbox centers the line box correctly. The remaining ~0.5px offset is inherent font metric asymmetry (visual cap-height center ≠ line box center) and not fixable without display: block (see below).

Button	max text shift
comfortable-sm (20px)	-0.5px (12.5% of visual gap)
spacious-sm (24px)	-0.5px
spacious-md / comfortable-lg (36px)	0px

Why inline-flex on span (not display: block)

display: block was thoroughly tested to enable text-box-trim (which only works on block containers). The problem: block context breaks icon+text layout — we lose flex gap and align-items: center for vertical centering of icons with text.

We could work around this by introducing startIcon/endIcon props (like MUI) to separate icons from text, enabling display: block on the text span. However, we intentionally keep the API close to native HTML — icons are just children, not special props. This is a deliberate design choice to avoid unnecessary API surface and keep the component composable:

// Our approach — close to native HTML
<Button><Icon data={add} /> Add item</Button>

// Alternative we chose not to do
<Button startIcon={<Icon data={add} />}>Add item</Button>

inline-flex preserves this children-based API and provides good enough centering via flexbox.

[vnys]

LGTM 👍🏻 We need to revisit the things that contradict the spacing and typography foundations (height is a function of the content + spacing) after the release, but I think this solves the issues we have for now.