Review 5181

#5181 — fix(NumberInput): track themed padding so the stepper column stays flush

freddymeta · merged · view on GitHub

Verdict: approve · 2 rounds

These records predate the Review Presentation format for the most part, so the problem/solution/API/theme-target/breaking slots are only filled where the review itself carried them. Reviews from 2026-08-23 on use the full template.

Round 1 — request-changes · 2026-08-19

Thanks — checked it in Chrome and the column stays flush with themed padding, in RTL and at 200% zoom.

The reason I'd hold it is the theme author's experience. This reads --_number-input-padding-inline/-block, so it only fires when someone writes exactly paddingInline or paddingBlock. A theme that writes padding: 14px 20px, or paddingBlockStart, gets the old broken gap back with nothing to tell them why — I hit 10/10/20px and a 12px top gap doing that. Silently working for one spelling of the same declaration is the footgun.

expand: 'container' already parses every spelling (Card, Dialog and Section use it). Could this go through that instead of a sixth hand-rolled entry? If it can't emit component-private vars, say so and I'll take that as ours to fix in the theming infra.

Also worth trimming the changeset: theming number-input borderRadius still leaves the steppers at the old radius — field 2px, steppers 10px in my run.

[Reviewed by Robohands, edited by the maintainer]

Round 2 — approve · 2026-08-20

Approving — this is exactly it, thank you.

Re-checked in Chrome against your previous push. Both spellings I named are closed, and the asymmetric case you found on your own was the sharpest one:

theme on number-input	before	now
padding: '14px 20px'	10px gap top+bottom	flush
paddingBlockStart: '12px'	8px gap top	flush
paddingBlock: '4px 12px'	calc(-1 * 4px 12px) invalid, margin dropped	flush
borderRadius: '2px'	field 2px, steppers 10px	both 2px

Flush in RTL too. derivedVarRegistry.test.ts:188 is the best part of the diff — that var-count bail is why the first version could land unchecked, and you found it unprompted.

Two things, neither yours and neither blocking:

• Physical longhands still escape — paddingTop: '14px' leaves a 10px gap. PADDING_PROPS (generateThemeRules.ts:121) lists only the logical spellings, so Card/Dialog/Section have it identically. Ours; I'll file it.
• Could you add a themed before/after screenshot to the description? It's the one thing a visual PR is asked to carry — I have shots if you want them.

The changeset reads long now; a trim to the big picture would be welcome, but don't hold the PR on it.

[Reviewed by Robohands, edited by the maintainer]