Repository navigation
fix(TimeInput, DateTimeInput): under a finger the AM/PM wheel follows the hour - #8179
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch | 237.3 kB (+1.3 kB) · 21.8 kB CSS (0) | — | — · — · — |
| DateTimeInput · Storybook | 169.0 kB (+1.3 kB) · 17.2 kB CSS (0) | 3,564 (−73) | ✓ · ✓ · ✓ |
| TimeInput · Storybook | 132.6 kB (+1.3 kB) · 14.5 kB CSS (0) | 1,606 (+355) | ✓ · ✓ · — |
1 more component
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateInput · Storybook | 147.1 kB (+135 B) · 15.5 kB CSS (0) | 5,102 (+68) | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.32 MB packed (+12.3 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 8 rows, theme-butter 1 row, theme-chocolate 1 row, theme-gothic 1 row, theme-matcha 1 row, theme-neutral 1 row, theme-stone 1 row, theme-y2k 1 row; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval needs an engineering owner's decision first: no current spec owns the sheet time wheels' rules (AM/PM following the hour, looping columns, the increment / timeIncrement minute interval, the turn onto a bound), and the diff adds draft spec:AST-094, whose FR1–FR23 have no owner approval. An engineering owner can record that decision by commenting /approve-spec b62f76f7eaa53408eab928ea2d987249376b6f48 on this pull request, and the author may do so as an engineering owner; otherwise, please say which rule or requirement should change. Whatever the decision, these need fixing:
TimeWheels.tsxandtimeWheels.tsbuild todist/TimeInput/TimeWheels.jsandtimeWheels.js, which collide on case-insensitive file systems and fail theregistry-contract (preview)check, so the rules module needs another name.- With
timeIncrement, DateTimeInput shows 11:58 PM as 12:00 AM and the next minute turn commits 12:15 AM on the same date, and withhasSecondsan interval resets the seconds column to 00 and the next turn drops the seconds. - The diff is 31,267 authored lines from the whole stack, so the earlier pull requests should land first and leave this one as the 1,780-line TimeWheels change.
[Automated review]
| type TimeColumn, | ||
| type TimeParts, | ||
| type TimeRow, | ||
| } from './timeWheels'; |
There was a problem hiding this comment.
./timeWheels and this file differ only by case once compiled: dist/TimeInput/TimeWheels.js and dist/TimeInput/timeWheels.js (plus their .d.ts and maps). They cannot coexist on the default macOS and Windows file systems, and CI's registry-contract (preview) consumer build already fails with "No matching export in …/dist/TimeInput/TimeWheels.js for import clampToBounds". Renaming timeWheels.ts and its test fixes it.
There was a problem hiding this comment.
Fixed: the rules live in timeWheelRules.ts, so no file differs from TimeWheels.tsx only by case.
| const minutes = time.hour * 60 + time.minute + time.second / 60; | ||
| const rounded = Math.round(minutes / interval) * interval; | ||
| return partsOf(rounded * 60); |
There was a problem hiding this comment.
With an interval above 1 this always returns seconds of 0, even with hasSeconds, while the doc comment says seconds drop only "when the picker has none". With hasSeconds and increment={15}, 10:15:30 shows :00 and the next hour or minute turn commits 10:xx:00. Rounding only the minutes and keeping time.second would match the comment.
There was a problem hiding this comment.
Fixed (see the later thread): seconds are kept with a seconds column; head 6e34dbc.
| time={parsedWheelTime} | ||
| hourFormat={hourFormat} | ||
| hasSeconds={hasSeconds} | ||
| minuteInterval={minuteIntervalOf(timeIncrement)} |
There was a problem hiding this comment.
With timeIncrement={15}, displayedTime shows 23:53–23:59 as 00:00, TimeWheels commits relative to that, and commitWheelTime joins the result to the unchanged date: Mar 15, 11:58 PM becomes Mar 15, 12:15 AM after one minute turn. Keeping the rounding inside the day here (or carrying the date) avoids the jump.
There was a problem hiding this comment.
Fixed (see the later thread): the rounding stays inside the day; head 6e34dbc.
| useIsomorphicLayoutEffect(() => { | ||
| latestRef.current = {shown, bounds, hourFormat, hasSeconds, onCommit, rows}; | ||
| }); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; the comment at line 164 does not. Naming the Wheel's rest callbacks, which fire from gesture and animation events outside render, would satisfy it.
b62f76f to
a5e1f03
Compare
There was a problem hiding this comment.
Approval needs an engineering owner's decision first: no current spec owns the sheet time wheels' rules (AM/PM following the hour, looping columns, the increment / timeIncrement minute interval, the turn onto a bound), and the diff adds draft spec:AST-094, whose FR1–FR23 have no owner approval. An engineering owner can record that decision by commenting /approve-spec a5e1f03a9e505b90e8a7fcde069c5b9f28af5df0 on this pull request, and the author may do so as an engineering owner; otherwise, please say which rule or requirement should change. Whatever the decision, these need fixing:
- With
timeIncrement, DateTimeInput shows 11:58 PM as 12:00 AM and the next minute turn commits 12:15 AM on the same date, and withhasSecondsan interval resets the seconds to 00 on the next turn (displayedTimeintimeWheelRules.ts). - The stack removes
dateInputTouchSizes.wheelItemSizeanddateInputTouchGeometry.wheelEdgePaddingfrom the released@astryxdesign/core/DateInput/tokens.stylexexport under patch changesets, so they need to stay (deprecated) until a minor. - The wheel's mouse-press guard and its six regression tests were deleted with no mouse test replacing them.
- The diff is 31,267 authored lines from the whole stack, so the earlier pull requests should land first and leave this one as the 1,780-line TimeWheels change.
[Automated review]
| } | ||
| const minutes = time.hour * 60 + time.minute + time.second / 60; | ||
| const rounded = Math.round(minutes / interval) * interval; | ||
| return partsOf(rounded * 60); |
There was a problem hiding this comment.
With an interval above 1 this always returns seconds of 0, even with hasSeconds, while the doc comment says seconds drop only "when the picker has none". With hasSeconds and increment={15}, 10:15:30 shows :00 and the next hour or minute turn commits 10:xx:00. Rounding only the minutes and keeping time.second would match the comment.
There was a problem hiding this comment.
Fixed (see the later thread); head 6e34dbc.
| : composeWheelTime({hour12: hour}), | ||
| ) | ||
| onCommit={time => | ||
| commitWheelTime(formatISOTime(time, hasSeconds)) |
There was a problem hiding this comment.
With timeIncrement={15}, displayedTime shows 23:53–23:59 as 00:00, TimeWheels commits relative to that, and commitWheelTime joins the result to the unchanged date: Mar 15, 11:58 PM becomes Mar 15, 12:15 AM after one minute turn. Keeping the rounding inside the day here (or carrying the date) avoids the jump.
There was a problem hiding this comment.
Fixed (see the later thread); head 6e34dbc.
| useIsomorphicLayoutEffect(() => { | ||
| latestRef.current = {shown, bounds, hourFormat, hasSeconds, onCommit, rows}; | ||
| }); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; the comment at line 164 does not. Naming the Wheel's rest callbacks, which fire from gesture and animation events outside render, would satisfy it.
| * column of values, not as a stack of buttons with air between them. At | ||
| * this height ~9 options are on screen. | ||
| */ | ||
| wheelItemSize: '28px', |
There was a problem hiding this comment.
@astryxdesign/core/DateInput/tokens.stylex is a published subpath (since 0.6.7), so removing wheelItemSize here, and wheelEdgePadding below, breaks any consumer that reads them, under patch changesets. Keeping both, marked @deprecated, until a declared minor, and moving wheelGeometry to a module that is not on the exports map, avoids the break.
There was a problem hiding this comment.
At 6e34dbc DateInput/tokens.stylex.ts exports the same names as on main.
| }); | ||
| }); | ||
| }); | ||
| // A finger or a mouse dragging a wheel goes through the gesture system: see |
There was a problem hiding this comment.
Wheel.test.tsx drives fingers, keys and clicks but no mouse drag, so the six deleted mouse tests have no replacement, including the one for a mouse press escaping the BottomSheet's pointer capture. With usePointerDragScroll gone, nothing else stops that press; a mouse drag and a wobbling mouse click on a wheel row inside the open sheet would keep the fixed defect covered.
There was a problem hiding this comment.
This PR deletes no tests from DateInputTouch.test.tsx at 6e34dbc; with a mouse the sheets keep today's column and its tests.
a5e1f03 to
0aefd1b
Compare
There was a problem hiding this comment.
Approval needs an engineering owner's decision first: no current spec owns the sheet time wheels' rules (AM/PM following the hour, looping columns, the increment / timeIncrement minute interval, the turn onto a bound), and the diff adds draft spec:AST-094, whose FR1–FR23 have no owner approval. An engineering owner can record that decision by commenting /approve-spec with this pull request's full head SHA, and the author may do so as an engineering owner; otherwise, please say which rule or requirement should change. Whatever the decision, these need fixing:
- With
timeIncrement={15}, DateTimeInput turns Mar 15, 11:58 PM into Mar 15, 12:15 AM after one minute row, and withhasSecondsan interval drops the seconds on the next turn (displayedTimeintimeWheelRules.ts). - The stack removes
dateInputTouchSizes.wheelItemSizeanddateInputTouchGeometry.wheelEdgePaddingfrom the released@astryxdesign/core/DateInput/tokens.stylexexport under patch changesets, so they need to stay, deprecated, until a minor. - With an interval of 10, 15 or 30 the minute column does not loop, although the changeset says it does.
- The wheel's six mouse tests were deleted with nothing replacing them, and the new Effects need comments naming what they synchronize.
- The diff is 31,115 authored lines from the whole stack, so the earlier pull requests should land first and leave this one as the 1,780-line TimeWheels change.
[Automated review]
| } | ||
| const minutes = time.hour * 60 + time.minute + time.second / 60; | ||
| const rounded = Math.round(minutes / interval) * interval; | ||
| return partsOf(rounded * 60); |
There was a problem hiding this comment.
With an interval above 1 this always returns seconds of 0, even with hasSeconds, while the doc comment says seconds drop only "when the picker has none". With hasSeconds and increment={15}, 10:15:30 rests the seconds column on 00, and one hour row commits 11:15:00. Rounding only the minutes and keeping time.second would match the comment.
There was a problem hiding this comment.
Fixed (see the later thread); head 6e34dbc.
| : composeWheelTime({hour12: hour}), | ||
| ) | ||
| onCommit={time => | ||
| commitWheelTime(formatISOTime(time, hasSeconds)) |
There was a problem hiding this comment.
With timeIncrement={15}, displayedTime shows 23:53–23:59 as 00:00, TimeWheels commits relative to that, and commitWheelTime joins the result to the unchanged date: Mar 15, 11:58 PM becomes Mar 15, 12:15 AM after one minute row. Keeping the rounding inside the day here (or carrying the date) avoids the jump.
There was a problem hiding this comment.
Fixed (see the later thread); head 6e34dbc.
| * inlines only literal constants, so the values are repeated here; a test | ||
| * holds them to the table. | ||
| */ | ||
| export const wheelGeometry = stylex.defineConsts({ |
There was a problem hiding this comment.
@astryxdesign/core/DateInput/tokens.stylex is a published subpath (since 0.6.7), and 0.7.4's types declare dateInputTouchSizes.wheelItemSize and dateInputTouchGeometry.wheelEdgePadding, which this file no longer has, under patch changesets. Keeping both, marked @deprecated, until a declared minor, and moving wheelGeometry to a module that is not on the exports map, avoids the break.
There was a problem hiding this comment.
At 6e34dbc DateInput/tokens.stylex.ts exports the same names as on main.
| useIsomorphicLayoutEffect(() => { | ||
| latestRef.current = {shown, bounds, hourFormat, hasSeconds, onCommit, rows}; | ||
| }); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; the comment at line 164 does not. Naming the Wheel's rest callbacks, which fire from gesture and animation events outside render, would satisfy it.
|
|
||
| /** Whether a looping wheel of `rowCount` rows can hide its seam. */ | ||
| function canLoop(rowCount: number): boolean { | ||
| return (rowCount * ROW_HEIGHT) / 2 > PARKED_ANGLE * GEOMETRY.radius; |
There was a problem hiding this comment.
This needs at least 11 rows to loop, so a minute column with an interval of 10, 15 or 30 (6, 4 or 2 rows) stops at 00 and at its last step, while the time-wheels-follow-uidatepicker changeset says the minute column loops. Repeating a short column's rows, or stating the exception in the changeset and docs, would make them agree.
There was a problem hiding this comment.
Right: short minute lists loop once the wheel does (#8366, which matches UIDatePicker's measured behaviour); a small follow-up PR stacks TimeWheels on it.
| * event — including the click. Measured before this: a click on a wheel row | ||
| * that wobbled more than a pixel selected nothing at all. | ||
| */ | ||
| it('keeps the press away from the sheet, which would capture the pointer', () => { |
There was a problem hiding this comment.
Wheel.test.tsx drives fingers, keys and clicks but no mouse, so the six deleted mouse tests have no replacement, including this one for a mouse press escaping the BottomSheet's pointer capture. A mouse drag and a wobbling mouse click on a wheel row inside the open sheet would keep the fixed defect covered.
| // Typed-field only: the wheels step by their own options and the sheet | ||
| // opens on tap, so neither reaches this surface. | ||
| increment: _increment, | ||
| increment, |
There was a problem hiding this comment.
This comment still says increment does not reach the sheet, but it now sets the minute wheel's rows; the matching comment in TouchDateTimeField.tsx ("Desktop-only: mobile uses wheels instead of typed arrow stepping") has the same problem for timeIncrement.
There was a problem hiding this comment.
Fixed: the comment now says the minute wheel steps by increment under a finger; head 6e34dbc.
0aefd1b to
d67340a
Compare
There was a problem hiding this comment.
Approval needs an engineering owner's decision first: no current spec owns the sheet time wheels' rules (AM/PM following the hour, looping columns, the increment / timeIncrement minute interval, the turn onto a bound), and the diff adds draft spec:AST-094, whose FR1–FR23 have no owner approval. An engineering owner can record that decision by commenting /approve-spec with this pull request's full head SHA, and the author may do so as an engineering owner; otherwise, please say which rule or requirement should change. Whatever the decision, these need fixing:
- With
timeIncrement={15}, DateTimeInput turns Mar 15, 11:58 PM into Mar 15, 12:15 AM after one minute row, and withhasSecondsan interval drops the seconds on the next turn (displayedTimeintimeWheelRules.ts). - The stack removes
dateInputTouchSizes.wheelItemSizeanddateInputTouchGeometry.wheelEdgePaddingfrom the released@astryxdesign/core/DateInput/tokens.stylexexport under patch changesets, so they need to stay, deprecated, until a minor. - The wheel's six mouse tests were deleted with nothing equivalent, and after a commit onto a
min/maxthat is off the interval the wheels rest on a rounded time outside the bounds. - The diff is 31,351 authored lines from the whole stack, so the earlier pull requests should land first, leaving the 2,016-line TimeWheels commit.
[Automated review]
| : composeWheelTime({hour12: hour}), | ||
| ) | ||
| onCommit={time => | ||
| commitWheelTime(formatISOTime(time, hasSeconds)) |
There was a problem hiding this comment.
With timeIncrement={15}, displayedTime shows 23:53–23:59 as 00:00, TimeWheels commits relative to that, and commitWheelTime joins the result to the unchanged date: Mar 15, 11:58 PM becomes Mar 15, 12:15 AM after one minute row. Keeping the rounding inside the day here (or carrying the date) avoids the jump; a 23:58 test would hold it.
There was a problem hiding this comment.
Fixed in 662b5c5: displayedTime keeps the rounding inside the day and inside the bounds, so with timeIncrement={15} 23:53–23:59 shows 23:45 and the wheels rest on a row that commits on the same date (timeWheelRules.test.ts).
| } | ||
| const minutes = time.hour * 60 + time.minute + time.second / 60; | ||
| const rounded = Math.round(minutes / interval) * interval; | ||
| return partsOf(rounded * 60); |
There was a problem hiding this comment.
With an interval above 1 this always returns seconds of 0, even with hasSeconds, while the doc comment says seconds drop only “when the picker has none”. With hasSeconds and increment={15}, 10:15:30 rests the seconds column on 00, and one hour row commits 11:15:00. Rounding only the minutes and keeping time.second would match the comment.
There was a problem hiding this comment.
Fixed in 662b5c5: the seconds are kept when the picker has a seconds column and dropped only when it has none (timeWheelRules.test.ts: 10:22:30 shows 10:15:30).
| * inlines only literal constants, so the values are repeated here; a test | ||
| * holds them to the table. | ||
| */ | ||
| export const wheelGeometry = stylex.defineConsts({ |
There was a problem hiding this comment.
@astryxdesign/core/DateInput/tokens.stylex is a published subpath (since 0.6.7), and this file no longer has dateInputTouchSizes.wheelItemSize or dateInputTouchGeometry.wheelEdgePadding, under patch changesets. Keeping both, marked @deprecated, until a declared minor, and moving wheelGeometry to a module that is not on the exports map, avoids the break.
| () => ({min: secondsOfISO(min), max: secondsOfISO(max)}), | ||
| [min, max], | ||
| ); | ||
| const shown = displayedTime(time, minuteInterval, hasSeconds); |
There was a problem hiding this comment.
The shown time is rounded without regard to min / max. With increment={15} and max="16:55", an AM/PM turn from 11:30 AM that clamps to 4:55 PM re-renders as 5:00 PM, a disabled hour row over a column of disabled minutes, while the field reads 4:55 PM. Rounding to the nearest step inside the bounds keeps the wheels on an enabled row.
There was a problem hiding this comment.
Fixed in 662b5c5: the rounding also stays inside min/max (16:55 under a 16:55 maximum shows 16:45), so an AM/PM turn lands on a row that can commit (timeWheelRules.test.ts).
|
|
||
| /** Whether a looping wheel of `rowCount` rows can hide its seam. */ | ||
| function canLoop(rowCount: number): boolean { | ||
| return (rowCount * ROW_HEIGHT) / 2 > PARKED_ANGLE * GEOMETRY.radius; |
There was a problem hiding this comment.
This needs at least 11 rows to loop, so a minute column with an interval of 6 or more (10, 15 and 30 among DateTimeInput's timeIncrement values) stops at 00 and at its last step, while the time-wheels-follow-uidatepicker changeset says the minute column loops. Repeating a short column's rows, or stating the exception in the changeset and docs, would make them agree.
There was a problem hiding this comment.
Right: with an interval of 6 or more, the minute column stops at its ends until #8289's short-column looping lands. The body says so.
| }); | ||
| }); | ||
| }); | ||
| // A finger or a mouse dragging a wheel goes through the gesture system: see |
There was a problem hiding this comment.
Wheel.test.tsx drives fingers, keys and clicks but no mouse, so the six deleted mouse tests have no equivalent, including the one for a mouse press escaping the BottomSheet's pointer capture (useSheetGestures.ts still captures non-touch pointers). A mouse drag and a wobbling mouse click on a wheel row inside the open sheet would keep that fixed defect covered.
There was a problem hiding this comment.
This head deletes no tests from DateInputTouch.test.tsx. With a mouse the sheets keep today's column, and its mouse tests stay as they are on main.
| onCommit, | ||
| rows, | ||
| }); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; the comment at line 164 does not. Naming the Wheel's rest callbacks, which fire from gesture and animation events outside render, would satisfy it (the same applies at Wheel.tsx:361 and :494).
| // Typed-field only: the wheels step by their own options and the sheet | ||
| // opens on tap, so neither reaches this surface. | ||
| increment: _increment, | ||
| increment, |
There was a problem hiding this comment.
The comment above still says increment does not reach this surface, but it now sets the minute wheel's rows (line 412); the Desktop-only comment above timeIncrement in TouchDateTimeField.tsx has the same problem.
There was a problem hiding this comment.
Fixed in 662b5c5: the comment now reads "Under a finger the minute wheel steps by it (UIDatePicker's minute interval); with a mouse today's wheels list every minute." TouchDateTimeField says the same for timeIncrement.
…st (#8276) ## User impact When a Popover, or anything built on `usePopover` such as a collapsed SideNav submenu, is dismissed within a frame of opening, focus jumps into the hidden popover's content. A quick Escape after a click or Enter is enough. Keyboard and screen-reader users are left on a control they can't see. The same race makes SideNav's "returns focus to the trigger when the collapsed submenu closes" test flaky on loaded runners, as seen on #8179: it took 1112 ms and ended with focus on the submenu's "General" link. ## Problem and solution fit - Why is focus lost? `usePopover` focuses its content in a `requestAnimationFrame` after it opens and never cancels that frame. - Why does the close not fix it? The close runs first. `useFocusTrap` leaves focus on the trigger because focus never entered the trap. Then the frame runs and focuses the first control in content that is no longer shown. - Why does it matter? Focus sits inside a closed layer, where nothing announces it and Escape no longer applies. The fix ties the frame to the open: closing or unmounting cancels it. ## Expected behavior and authority `component:Popover` (current): - Automatic focus moves focus into the content "for each accepted opening". - Dismissal returns focus to the trigger; `Popover.test.tsx` covers "returns focus to the trigger when closed via Escape". An opening that has already closed has no content to focus. ## Smallest restoration - `usePopover`'s auto-focus effect returns `() => cancelAnimationFrame(frame)`. - The per-open reset of the skip-auto-focus preference is unchanged. - No API, timing or visual change. ## Evidence - Reproduction before the change: hold animation frames, open the collapsed SideNav submenu, press Escape, then run the held frame. Focus lands on the "General" link inside the closed submenu. The Popover equivalent lands on its content button. - Result after the change: in both cases the trigger keeps focus. - Representative unchanged path: opening still focuses the first content control in the next frame. Escape after that still returns focus to the trigger, now asserted without a timing wait. - Regression tests: - "does not move focus into a popover that closed before its first frame" (Popover); - "keeps focus on the trigger when the submenu closes before its first frame" (SideNav). Both fail with the fix reverted. ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. - [x] Public text and artifacts contain no internal Meta context. ## Testing - `pnpm vitest run --project ui` on Popover, SideNav, and every directory with a `usePopover` consumer: Breadcrumbs, Chat, ComplexSelector, DateInput, DateRangeInput, DateTimeInput, DropdownMenu, hooks, PowerSearch, Selector, TabList, TopNav, Typeahead, lab Schedule. 115 files, 3183 tests passed. - With `usePopover.tsx` reverted, the two new tests fail and the reworked SideNav focus-return test still passes. That test is now deterministic: it flushes the open frame, asserts focus is on "General", presses Escape, and asserts the trigger has focus immediately, with no `waitFor`. - `pnpm -F @astryxdesign/core typecheck`, eslint and prettier on the touched files, `check:changesets`, and pre-commit `check:repo`.
a7b3537 to
ed7552d
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
Size. This pull request's own change is 2,684 authored lines, 1,734 of them tests, including the 235-line The DEC-2 wording and the haptic gap are fixed in #8159 at 69695d1. The row builders are shared here at ed7552d: the mouse columns build their rows from |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about this head's spec changes, and four fixes are needed regardless.
An engineering owner needs to approve or amend, for this exact head, the record text it adds or changes: TimeInput FR7's rule that the AM/PM wheel follows the hour under a finger, the rewrite of DateInput DEC-2 (flat rows under a finger, tilted rows under a mouse) and its Non-goals line, DateInput FR14's tap-spin role sentence, and new DateInput FR17 (keys step in time order) and FR18 (no accent mark while a wheel moves). The earlier approval covered the previous head only. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking:
- Under a finger, a focused wheel paints no keyboard focus ring, because the column's
mask-imageclips its own outline (PickerWheel.tsx:90-91,:733); move the mask to an inner wrapper so keyboard users can see focus. - The layout Effect in
TimeWheels.tsx:137-146copies render values into a ref thatrestOndoes not need, since both wheels already call the latestonChange; delete it and read the render values directly. - The size report compares only against the parent branch; report each changed component against
mainand justify the touch-only JavaScript the stack adds, as the component size rule asks. - The diff is 10,066 authored lines, over the 8,000-line limit for an owner-directed batch, and 7,082 of them come from #8178; land #8178, rebase, and keep this change under 2,000 lines or show why it cannot be split.
[Automated review]
| // to fully opaque at the band and back. The scrolling column fades them the | ||
| // same way under reduced motion. A static mask: nothing paints it again as | ||
| // the strip moves. | ||
| maskImage: |
There was a problem hiding this comment.
This mask is on the focusable column, so it also clips the column's own focus outline (2 px at a 3 px offset, outside the border box). Under touch emulation the focused Hour wheel matches :focus-visible and paints no ring, against interaction-modality INV1. Moving the mask to an inner wrapper around the band and the strip keeps the fade and lets the ring show.
| // Synchronizes the Wheels' rest callbacks with this render. The Wheel calls | ||
| // them from gesture and animator events, outside render. So they read the | ||
| // latest time, bounds and rows through this ref. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
Both wheels already call the latest onChange at rest (PickerWheel reads it from its own latest-props ref, the scrolling column refreshes its settle callback every render), so restOn's closure holds the same values this Effect copies. Deleting latestRef and this Effect, and reading the render values in restOn, removes an Effect that synchronizes no external system.
| const RUBBER_BAND_DIMENSION = picker.geometry.rubberBandDimension * UIKIT_SCALE; | ||
|
|
||
| const styles = stylex.create({ | ||
| // Today's column (Wheel.tsx): the same box, band and rows, moved by the |
There was a problem hiding this comment.
Non-blocking: "Today's column" names nothing once this merges; "The scrolling column (Wheel.tsx)" says the same. The same applies at line 95 and in useWheelListbox.ts line 44.
ed7552d to
986b0bb
Compare
|
/approve-spec 986b0bb |
github-actions
Bot
removed
the
needs:spec-owner-review
|
This head. Rebased onto Size (INV6), main cae1f66 → this head: TimeInput 90.1 → 130.5 KB, DateTimeInput 114.9 → 156.3 KB, PowerSearch 183.6 → 225.0 KB, MobilePowerSearch 168.5 → 209.6 KB, DateInput 103.0 → 144.2 KB. Most of it is #8159's shared touch runtime, which a page pays once (spec owners' ruling, Nov 2). This pull request's own part, #8178 → this head: TimeInput +1.2 KB, DateTimeInput +1.3 KB, PowerSearch +1.3 KB, MobilePowerSearch +1.0 KB, DateInput +0.2 KB, each under INV6's 2 KB flag for touch-only JavaScript; CSS unchanged. Lines. This pull request's own change is 2,684 authored lines, 1,750 of them tests, including the 235-line |
There was a problem hiding this comment.
Changes requested. An engineering owner has approved the record changes at this head, so what remains is scope and four fixes:
- Land #8178 and rebase so this diff holds only the time-wheel change: it is 9,429 authored lines, over the 8,000-line limit for an owner-directed batch, and about 6,400 of them are #8178's separable commits.
- The changeset's second bullet (
.changeset/time-wheels-follow-uidatepicker.md:8) announces the rest pastmin/maxas new, but the base sheet already travels to the bound, turns the other columns onto it and leaves an untouched value alone, so drop the bullet or narrow it to a bound that falls between whole minutes. - Complete the
component-style-authoringINV6 size report: add each changed component's CSS delta againstmain, and justify the wheel's own touch-only JavaScript (PickerWheel,wheelPhysics,wheelStrip, about 5 KB gzip apart from the shared touch modules), which the size comment attributes only to the shared runtime. restingTimeandclampToBoundsintimeWheelRules.tsre-implementclampTimefromutils/timeParser.ts, and the two already disagree for a bound between whole minutes (withmin="09:00:30"the finger path commits 9:01 and the mouse columns commit nothing), so extendclampTimeand call it from both paths.
The AM/PM rule itself works: under a finger, 11:30 AM plus one hour row now commits 12:30 PM, and the date and time input suites pass.
[Automated review]
| [fix] Under a finger, the time wheels in the TimeInput and DateTimeInput bottom sheets now follow iOS's date picker: | ||
|
|
||
| - The AM/PM column follows the hour. Turning the hour from 11 to 12 now gives 12 PM, not 12 AM, and the AM/PM column turns onto PM. From 12 PM to 1 it stays PM. | ||
| - A wheel that comes to rest past `min` or `max` goes on to the nearest time inside them and commits it there, and the other columns turn onto it. Opening the sheet never changes the value: a time outside the bounds stays as it is, and an empty field stays empty. |
There was a problem hiding this comment.
The base sheet already does this under a finger: a wheel that rests past min travels to the bound and commits it, the other columns turn onto it, and opening never changes an untouched value (the base WheelRest.a11y.chromium.spec.ts and Wheel.test.tsx cover all three). Drop this bullet, or narrow it to what changed: under a finger, a bound that falls between whole minutes now commits the nearest whole minute inside it.
| } | ||
|
|
||
| /** `time` brought inside the bounds: past a bound, onto it. */ | ||
| export function clampToBounds(time: TimeParts, bounds: TimeBounds): TimeParts { |
There was a problem hiding this comment.
This re-implements clampTime (utils/timeParser.ts), which the mouse columns and the finger path's commit still use, and the two already disagree: with min="09:00:30" and no seconds column, restingTime gives 9:01 while clampTime gives 09:00, which isTimeInRange rejects. Extend clampTime with the rounding onto the nearest whole minute inside the bounds, call it from restingTime, and delete this copy.
| }); | ||
| }, [hourFormat, max, min, parsedWheelTime.hour]); | ||
|
|
||
| // The mouse columns' rows come from the same rules as the finger wheels'. |
There was a problem hiding this comment.
Non-blocking: this comment narrates the imported calls below it; consider deleting it (the same line sits in TouchDateTimeField.tsx).
|
/approve-spec 05396c7 |
There was a problem hiding this comment.
Changes requested. An engineering owner approved the AM/PM rule at this head, and the tests and an emulated-phone check show 11:58 AM turning to 12:58 PM under a finger. Two things remain:
- The diff against
mainis 9,501 authored lines, about 6,400 of them from the still-open #8159 and #8178, which is over the 8,000-line limit for an owner-directed batch; land those two, rebase so this pull request holds only its own ~3,100 lines, and then show it qualifies or split it. - The size report that
component-style-authoringINV6 requires gives no CSS delta againstmain(PickerWheel.tsxadds StyleX rules to all three date and time inputs) and no justification for the wheel's own touch-only JavaScript; post both, or rely on your own-part report once the rebase leaves only this change.
Non-blocking: the comments at TouchTimeField.tsx:224, TouchDateTimeField.tsx:558 and Wheel.tsx:241-242 restate the isTouch branch below them and can go.
[Automated review]
| const mergedInputRef = useMergedRefs(ref, inputRef); | ||
| const inputGroup = useInputGroup(); | ||
| const [isSheetOpen, setIsSheetOpen] = useState(false); | ||
| // A finger gets UIDatePicker's wheels; a mouse gets the scrolling columns. |
There was a problem hiding this comment.
Non-blocking: this comment restates the isTouch ? <TimeWheels … /> : <>…</> branch below (architecture:knowledge-contracts INV29). Consider deleting it.
| ); | ||
| const [activePanel, setActivePanel] = useState<TouchDateTimePanel>('date'); | ||
| const [isWheelOpen, setIsWheelOpen] = useState(false); | ||
| // A finger gets UIDatePicker's time wheels; a mouse gets the scrolling columns. |
There was a problem hiding this comment.
Non-blocking: as in TouchTimeField.tsx, this restates the isTouch branch below (INV29). Consider deleting it.
|
Size (INV6), main cae1f66 → 05396c7, gzip. JavaScript: TimeInput 90.1 → 130.5 KB, DateTimeInput 114.9 → 156.3 KB, PowerSearch 183.6 → 224.9 KB, MobilePowerSearch 168.5 → 209.6 KB, DateInput 103.0 → 144.2 KB. CSS: TimeInput +73 B, DateInput +51 B, DateTimeInput +49 B, PowerSearch +47 B, MobilePowerSearch +0 B, all from #8159's wheel styles. Why. Most of that JavaScript is #8159's shared touch runtime, which a page pays once. The spec owners kept touch code eager, with no lazy loader, so a gesture never waits for a load (Nov 2), as for Slider. This pull request's own part, #8178 → this head: TimeInput +1.2 KB, DateTimeInput +1.3 KB, PowerSearch +1.3 KB, MobilePowerSearch +1.0 KB, DateInput +0.2 KB, each under INV6's 2 KB flag; CSS unchanged. Lines. After #8159 and #8178 land, this pull request is its own 2,684 added and 374 deleted lines, 1,750 of the additions tests, within B4's 8,000-line batch. |
|
/approve-spec 12a0c16 |
There was a problem hiding this comment.
Changes requested. An engineering owner has approved the record changes on this exact head. The AM/PM rule, the whole-minute clampTime fix and their tests are ready.
What remains:
- Scope: against
mainthis pull request is 9,497 authored lines, and about 6,420 of them are the still-open #8159 and #8178. That puts it over the 8,000 lines an owner-directed batch allows. Merge those two, then rebase so this diff is its own change, about 3,100 lines.
Two smaller points: the INV6 size report should be re-posted for this head once the rerun lands. datePickerTouch.ts:8-9 says the sheets import touch behaviour only from that module, but they also reach PickerWheel through Wheel. Reword or drop that line (non-blocking).
[Automated review]
| * @position The pickers' touch layer. The sheets import touch behaviour from | ||
| * here only, so a desktop build can replace this one module. |
There was a problem hiding this comment.
Non-blocking: the sheets also reach the touch column through Wheel (Wheel.tsx imports PickerWheel and renders it under a finger). So this module is not their only touch import, and replacing it alone would not drop the touch code from a desktop build. Suggest: "The time wheels the sheets render only under a finger."
|
/approve-spec c821d04 |
There was a problem hiding this comment.
Changes requested: one scope fix remains. An engineering owner has approved the record changes on this exact head, and the AM/PM rule and the whole-minute clampTime fix otherwise look ready.
- Scope: against
mainthis pull request is 9,558 authored lines, about 6,486 of them from the still-open #8159 and #8178, which is over the 8,000 lines an owner-directed batch allows; land those two and rebase so this diff is its own change (about 3,100 lines), or declare the three as one stack with the sameStack: #8159 → #8178 → #8179line in each description.
Non-blocking: the header of packages/core/src/DateInput/datePickerTouch.ts says the sheets import touch behaviour only from that module, but Wheel.tsx imports PickerWheel directly (inline note). After a rebase, the records need a fresh /approve-spec on the new head.
[Automated review]
| * @position The pickers' touch layer. The sheets import touch behaviour from | ||
| * here only, so a desktop build can replace this one module. |
There was a problem hiding this comment.
Non-blocking: the sheets also render Wheel, which imports the touch column PickerWheel itself (Wheel.tsx:54, :248), so this module is not their only touch import. Shorter and true: "The time wheels the sheets render only under a finger."
|
/approve-spec 89e3bb3 |
There was a problem hiding this comment.
Recommend approval. An engineering owner has approved the record changes on this exact head. The AM/PM rule and the whole-minute clampTime fix come with tests that fail before the change and pass after it, under a finger and with a mouse.
- This diff includes #8178's commits, so it merges after #8178 or together with it.
- The
Stack:line in #8209, #8200 and #8206 lists #8159 first and differs from this description's, and #8209 does not contain this head. With identical lines, the stack could be reviewed as one. - Two optional comment fixes are noted inline, in
datePickerTouch.tsandPickerWheel.tsx.
[Automated review]
| * @position The pickers' touch layer. The sheets import touch behaviour from | ||
| * here only, so a desktop build can replace this one module. |
There was a problem hiding this comment.
Non-blocking: the sheets also render Wheel, which imports and renders the touch column PickerWheel itself (Wheel.tsx:54, :248), so replacing this module would not remove the touch path. Suggested: "The sheets import TimeWheels from here. Wheel.tsx imports the touch column itself."
| // A spin turns only onto an enabled row (canSpinTo). So its arrival commits | ||
| // that row, and nothing turns on past it. It reports the row it was asked | ||
| // for, as the scrolling column does. |
There was a problem hiding this comment.
Non-blocking: spinTo resolves a boolean, not a row. Suggested: "It resolves true only when it arrives on that row, as the scrolling column does."
|
/approve-spec 894ea24 |
There was a problem hiding this comment.
Recommend approval. An engineering owner approved this head's TimeInput FR7 and DateInput record text, and the code matches it: under a finger the AM/PM wheel now follows the hour (11:58 AM to 12:58 PM), and the mouse sheet is unchanged.
- The finger and whole-minute clamp tests fail on
mainand pass here, and an emulated Pixel 7 shows 12:58 PM wheremaingives 12:58 AM. - This diff still carries #8178, so it merges after #8178 or together with it.
- Optional: the
datePickerTouch.tsheader and thespinTocomment inPickerWheel.tsxstill describe the code inaccurately (see the earlier inline suggestions), and FR7 could say how the AM/PM change reaches a screen reader.
[Automated review]
The time wheels turn a column by themselves when another column's change moves its value, so the Wheel offers that turn on its ref: `spinTo(index)` (`WheelHandle`). Under a finger the wheel turns onto the row on the tap spin, then commits it as a rest would; under a fine pointer the scrolling column commits the row at once, and the new value scrolls it there. Each reports whether the row was committed: a disabled row or an index outside the rows resolves false at once, and a finger, another turn or the wheel's unmounting cutting the turn short resolves false.
the hour, as UIDatePicker's does Under a finger the TimeInput and DateTimeInput sheets render TimeWheels, through the pickers' touch layer (datePickerTouch.ts). The half of the day changes only when the hour's move passes between 11 and 12: 11 AM turned to 12 gives 12 PM, and the AM/PM column turns onto the new half on the wheel's own spin (component:DateInput FR14). The hour column does not loop, so 12 counts as next to 1: 12 PM turned to 1 stays 1 PM, and 1 PM turned to 9 stays 9 PM. A column that comes to rest past min or max goes on to the nearest time inside them and commits it there, the other columns turning onto it; an untouched time outside the bounds stays as it is. A turn the wheels start by themselves is not a person's rest: its arrival commits nothing, even when the owner kept the old time. With a mouse the sheets keep today's independent columns. The columns do not loop, and the minute wheel lists every minute.
…column TimeInput FR7 records the rule the spec owners asked for at the B4 head. On the non-looping hour column, 12 counts as next to 1. Moving between 12 and 1–6 keeps the half of the day; any other move flips it when it passes between 11 and 12. It cites DateInput FR14's turn-onto rule, and the FR7 verification row names the tests.
|
/approve-spec e51ebcc |
There was a problem hiding this comment.
Recommend approval, pending CI. An engineering owner approved this head's component:TimeInput FR7 text, and the code matches it: under a finger the AM/PM wheel now follows the hour (11:58 AM to 12:58 PM), and the mouse sheet is unchanged.
- Approved pending CI: CI has not run on this head yet. The pull request is a draft on a fork branch, so CI runs once it is marked ready for review and a maintainer adds the
ci:runlabel. - The finger and whole-minute clamp tests fail on
mainand pass here, and an emulated Pixel 7 shows 12:58 PM wheremaingives 12:58 AM. - Optional: the
datePickerTouch.tsheader and thespinTocomment inPickerWheel.tsxdescribe the code inaccurately (see the inline suggestions), and FR7 could say how the AM/PM change reaches a screen reader.
[Automated review]
| * @position The pickers' touch layer. The sheets import touch behaviour from | ||
| * here only, so a desktop build can replace this one module. |
There was a problem hiding this comment.
Optional (comment accuracy): Wheel.tsx imports and renders the touch column PickerWheel itself, so replacing this module would leave the touch path in. Suggested:
* @position The pickers' touch layer. The sheets import TimeWheels from
* here. Wheel.tsx imports the touch column itself.| // A spin turns only onto an enabled row (canSpinTo). So its arrival commits | ||
| // that row, and nothing turns on past it. It reports the row it was asked | ||
| // for, as the scrolling column does. |
There was a problem hiding this comment.
Optional (comment accuracy): spinTo resolves a boolean, not a row. Suggested:
// A spin turns only onto an enabled row (canSpinTo). So its arrival commits
// that row, and nothing turns on past it. It resolves true only when it
// arrives on that row, as the scrolling column does.
Batch: B4 · Wheel / picker (DateInput, TimeInput, DateTimeInput)
Stack: #8179 → #8209 → #8200 → #8206
Depends on #8178. A pure rebase of 12a0c16 onto main: its own product diff is byte-identical, so the phone check that passed there carries. Review only this pull request's change: vjeux/astryx@fdc1ee1...894ea24
User impact
Under a finger, the hour wheel in the TimeInput and DateTimeInput sheets moves without the AM/PM column. Turning 11 AM to 12 gives 12 AM, where UIDatePicker gives 12 PM.
Problem and solution fit
TimeWheels, which applies TimeInput FR7 (timeWheelRules.ts) and turns the AM/PM column on the Wheel's spin before the commit.clampTime).Expected behavior and authority
/approve-spec.component:DateInputFR13, FR14 and FR17 for the rest, the turn and the keys. A looping hour column follows once a record covers it.Smallest restoration
Its first commit, by #8159's author, restores the Wheel's internal
spinTo, which only these wheels call. ThenTimeWheels.tsxandtimeWheelRules.ts(new, internal) behindDateInput/datePickerTouch.ts;TouchTimeField.tsxandTouchDateTimeField.tsx;clampTimeinutils/timeParser.ts; TimeInput FR7 and its verification row; theBottomSheetWheelsstory.Writing rubric results
Over FR7's changed sentence and its verification row: R1 narration, R2 research, R4 rejected alternatives, R5 change references, R6 open questions: none. R3: test names only.
Evidence
TimeWheels.interaction.browser.spec.tsthe AM/PM column turns onto PM frame by frame, also through a blocked main thread, under Reduced Motion and caught mid-turn.TimeWheels.test.tsx"keeps PM from 12 to 1" and "keeps PM from 1 back to 12", and thetimeWheelRules.test.tscrossing cases.Scope
Testing
This head, e51ebcc, rebases 894ea24 onto main ed33727, where #8178 merged as 826d893. The only content change is the one #9811's lint rule asks for:
TimeWheels.interaction.browser.spec.tsimportsexpectandtestfrom@astryxdesign/a11y-spec/playwright.git range-diffshows commits 1 and 3 equal and commit 2 changed only by hunk context and the spec's import; the added and removed lines match 894ea24's in every other file.pnpm install --frozen-lockfile0;pnpm -F @astryxdesign/core typecheck0;CI=true pnpm lint0;check:knowledge0;check:changesets0;pnpm review:pre0; the DateInput, TimeInput, DateTimeInput, Calendar and utils unit tests: 1,980 passed.TimeWheels.interaction.browser.spec.tsagainst Storybook built at e51ebcc: Chromium 6 passed; WebKit 5 passed, 1 skipped (the blocked-main-thread case: WebKit's screencast clock runs about 100 ms off the page's, so the iOS simulator is that case's WebKit evidence).