(fix) O3-5842: Match Carbon's control height in the date pickers - #1822
Draft
dkayiwa wants to merge 5 commits into
Draft
(fix) O3-5842: Match Carbon's control height in the date pickers#1822dkayiwa wants to merge 5 commits into
dkayiwa wants to merge 5 commits into
Conversation
OpenmrsDatePicker rendered its input group at 41px where Carbon's md-size inputs are 40px, so a date picker sitting next to a TextInput or ComboBox in a form row put the field bottoms and underlines a pixel apart. The height and the 1px bottom border were on different elements: the group carried the border but had no height of its own, and the height came from the input wrapper and the trigger button inside it. With auto height, the border was added on top of the 40px children, giving 41px. Carbon puts both on the same border-box element (see `.cds--text-input`, `.cds--date-picker__input`), so the border sits inside the height. Do the same here: the group now owns the height and the border, and its children fill it. The height is read from Carbon's layout size tokens through the `cds--layout--size-*` class the components set, which is how Carbon's own inputs are sized, so we follow Carbon if those values ever change. This also fixes the sm and lg variants (33px and 49px against Carbon's 32px and 48px) and the same problem in OpenmrsDateRangePicker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwrMcxXwSx8rPmMZu2VigK
|
Contributor
|
Size Change: -317 kB (-3.9%) Total Size: 7.81 MB 📦 View Changed
ℹ️ View Unchanged
|
Now that the input group sizes its own trigger button, the only remaining user of this class is the calendar popover's previous/next month navigation, which is a fixed 2.5rem square and deliberately does not scale with the field. Keeping "Md" in the name suggests it is the md variant of a set of per-size classes that no longer exists, and that it tracks the field's size, which it does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwrMcxXwSx8rPmMZu2VigK
…ntext The height fix takes the control height from Carbon's layout size tokens, but it stamped `cds--layout--size-md` on the input group whenever no `size` prop was given. That class does not just select a size, it *sets* the layout context for the subtree it is on, so an unconditional class opts the picker out of any surrounding context. Measured in Chrome: inside a `cds--layout--size-sm` container an unsized Carbon TextInput is 32px while an unsized picker stayed at 40px, which is the same kind of mixed-row misalignment O3-5842 is about. Carbon's own TextInput only emits the class when a size is passed, and leaves the fallback to `layout.use()`'s `$default` in the stylesheet. Do the same: drop the `md` default from both components and return no class for an absent size. An unsized picker outside any layout context is still 40px, because `.inputGroup` already declares `$default: 'md'`; inside a sm context it is now 32px, matching the Carbon inputs next to it. This also removes the `size.length === 0` branch, which existed only to force the md class for an empty size. An empty size is now falsy in exactly the way Carbon treats it. Measured in Chrome via Storybook, single and range pickers, LTR and RTL: sm 32px, md 40px, lg 48px, bottom border on the same y as `.cds--text-input` at every size; unsized 40px standalone and 32px inside a sm context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwrMcxXwSx8rPmMZu2VigK
`getLayoutSizeClass` interpolated whatever it was handed straight into the class
name. The `size` prop is typed `'sm' | 'md' | 'lg'`, but the framework is
consumed from untyped JavaScript, and feeding it the range a caller can actually
pass shows three problems:
size={40} -> cds--layout--size-40
size="Md" -> cds--layout--size-Md
size="md cds--layout--size-lg" -> two classes; the picker renders at lg
The first two are inert tokens that read like Carbon classes but match nothing,
so the group silently falls back to the `md` default while the DOM claims
otherwise. The third is worse: a value containing whitespace emits a second
class, and the later `cds--layout--size-*` wins the cascade, so a caller asking
for `md` gets `lg`.
Check the value against the three supported steps instead. In-contract callers
are unaffected; everything else now behaves exactly like an absent size and
inherits the surrounding layout context. Carbon's own `TextInput` interpolates
naively, so `size="xl"` no longer clamps to `lg` via Carbon's `$max` and instead
falls back to the layout context — an off-contract input either way, and the
predictable arm is the better one.
`undefined`, `null` and `''` were already handled and still are; no input
produces a `cds--layout--size-undefined`, and none of them crash.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwrMcxXwSx8rPmMZu2VigK
denniskigen
approved these changes
Jul 31, 2026
Member
There was a problem hiding this comment.
LGTM! Thanks, @dkayiwa. Approving so the docs bot can regenerate and clear drift.
| // has to survive whatever a caller actually passes. Nothing outside the three supported steps | ||
| // may reach the DOM as a `cds--layout--size-*` token: the group must stay unsized and inherit | ||
| // the layout context instead. | ||
| it.each([ |
Member
There was a problem hiding this comment.
Nit: this invalid-input matrix now runs three times: once against getLayoutSizeClass directly in utils.test.ts, and once through a full component render in each picker's test file. Since both components just delegate to the helper, utils.test.ts can own the full matrix and a single representative case here (say, the whitespace-injection one) would prove the wiring. Would cut about 40 lines across the two component test files with no coverage loss. Can be ignored.
|
|
||
| // See the note in datepicker.test.tsx: untyped JavaScript callers can pass anything, and none | ||
| // of it may reach the DOM as a `cds--layout--size-*` token. | ||
| it.each([ |
|
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.
Requirements
size, so there is nothing to update there. The type signatures are unchanged, but see On the API surface below: the runtime default ofsizechanged, and the generated docs do need a regeneration.Summary
OpenmrsDatePickerrendered its input group at 41px where Carbon's md-size text inputs are 40px, so a date picker next to aTextInputorComboBoxin a form row put the field bottoms and underlines a pixel apart (reported from the filter row of the audit log app).Root cause. The height and the 1px bottom border lived on different elements.
.inputGroupcarriedborder-block-end: 1px solidbut had no height of its own; the height came from the children (.inputWrapperMd/.flatButtonMd, both2.5rem). Withblock-size: auto, the border is added on top of the 40px children, so the group measures 41px. Carbon puts the height and the border on the sameborder-boxelement — see.cds--text-input(block-size: layout.size('height')+border-block-end: 1px solid $border-strong) and.cds--date-picker__input— so the border sits inside the 40px.Fix. The group now owns both the height and the border, and its children fill it (
block-size: 100%). The height is read from Carbon's layout size tokens vialayout.use('size', $default: 'md', $min: 'sm', $max: 'lg')/layout.size('height'), keyed off thecds--layout--size-*class the components set — exactly the mechanismcds--text-inputuses (TextInput.jsadds[`${prefix}--layout--size-${size}`]: size). Nothing is hardcoded against Carbon's sizing, so a Carbon upgrade that changes those tokens carries over automatically.This also fixes the
smandlgvariants, which were 33px and 49px against Carbon's 32px and 48px, and the same bug inOpenmrsDateRangePicker.Removed / renamed classes.
inputWrapperSm/Md/Lg,inputsWrapperSm/Md/LgandflatButtonSm/Md/Lgare all gone — the group sizes its own children now. The one rule that had to survive is the calendar popover's previous/next-month buttons, which are a fixed2.5remsquare and deliberately don't track the field size; that rule was renamedflatButtonMd→.calendarNavButtonso the name stops implying it is the md member of a set that no longer exists. None of the removed classes had consumers outside this directory, and none of them were part of the exported sass surface.Size is inherited, not defaulted.
sizeno longer defaults to'md'in the components. Emittingcds--layout--size-mdunconditionally would have opted the picker out of any surrounding Carbon layout context: inside acds--layout--size-smcontainer an unsizedTextInputis 32px, but an unsized picker would have stayed 40px. With no class, the size comes from the ancestor context, falling back to the$default: 'md'in.inputGroup'slayout.use()— so an unsized picker outside any layout context is still 40px, and inside one it now matches its neighbours. This mirrorsTextInput, which also only emits the class when a size is passed.Values outside
sm/md/lgare treated as "no size" rather than interpolated into the class name. The prop is typed, but plenty of callers are untyped JavaScript, and interpolating whatever arrives produced tokens that look like Carbon classes but match nothing (size={40}→cds--layout--size-40), or — for a value containing whitespace — a second class that wins the cascade, sosize="md cds--layout--size-lg"rendered atlg.Measured heights (Chromium,
deviceScaleFactor: 1;getBoundingClientRect().heightof the picker's[role="group"]against a real CarbonTextInput/ComboBoxof the same size rendered in the same document, with the styleguide's_all.scssloaded):OpenmrsDateRangePicker)Verified three ways with the same numbers:
yarn storybook,Components/DatePickerandComponents/DateRangePickerstories, driven with Playwright), in LTR and RTL.TextInputandComboBox.yarn run:shellwithOMRS_PROXY_TARGETpointed at a local OpenMRS 8081) so the real frontend modules from the instance's own importmap render against this styleguide, then Patient Registration and Appointments were driven with Playwright. On Patient Registration the date-of-birth picker measures 41px onmainand 40px with this branch, against the page'scds--text-inputfields at 40px; every element below the picker shifts up by exactly 1px between the two builds. The group's computed--cds-layout-size-height-localisclamp(max(0px, 2rem), 2.5rem, min(999999999px, 3rem))— i.e. the height really is coming from Carbon's layout token, clamped to sm..lg, and not from a hardcoded value.No regressions found. In the browser, at all three sizes: the calendar opens, renders 35 day cells, a date can be picked, the popover closes and the value lands in the input, and the group height holds after a value is set. The invalid, disabled and default-value stories still render correctly (error outline, warning icon and error text intact); text and the calendar icon remain vertically centred. The popover is portaled, so it cannot inherit the group's layout class.
Screenshots
Not attached — the change is a 1px height correction; the measurements above are the meaningful evidence. Before, in a mixed row, the date picker's underline sat 1px below the neighbouring inputs'; after, all the underlines share a baseline.
Related Issue
https://openmrs.atlassian.net/browse/O3-5842
Other
On the tests. The package runs under vitest with happy-dom, which has no layout engine and cannot report a rendered height, so there is no honest way to assert "40px" in the unit tests. Instead the
size propsuites indatepicker.test.tsxanddate-range-picker.test.tsxassert the mechanism that produces the height: the group carriescds--layout--size-{sm,md,lg}when a size is given, and carries nocds--layout--size-*class at all when one isn't, so that it inherits the surrounding layout context. The same suites cover the full range an untyped caller can actually pass —null,'','xl','Md', a number, an object, and a whitespace-injection string — and assert none of it reaches the DOM as a layout class.utils.test.tscoversgetLayoutSizeClassdirectly over the same range. The height itself was verified in a real browser as described above.yarn testfor@openmrs/esm-styleguidepasses (289 passed, 1 skipped), as doeslintandprettier --checkon the changed files.On the API surface. No exported type changed and no new symbol is exported (
getLayoutSizeClassis internal tosrc/datepicker/and is not re-exported fromdatepicker/index.tsx). Two things did change that a reviewer should see explicitly:size(previously always'md', now inherited — see above). Callers that relied on a picker staying 40px inside a non-md layout context will now see it match that context, which is the intended behaviour.sizeinOpenmrsDatePickerPropsandOpenmrsDateRangePickerProps, which means the committed API docs are stale for this branch.The
driftcheck is therefore red on purpose and cannot be cleared from this branch: it requires theDocs / Regeneratedcheck run, which only the bot posts, and the bot only runs after an approving review from a write-access reviewer. Committing regenerated docs by hand does not help — the same check hard-fails on any manual edit underpackages/framework/esm-framework/docs/. Please just approve; the bot will push the docs commit anddriftwill go green.skip-docswould be the wrong lever here, because the docs genuinely do drift.