Repository navigation
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📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsDateInput (@astryxdesign/core) · View in Storybook
DateTimeInput (@astryxdesign/core) · View in Storybook
TimeInput (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/DateInput | +29.0KB |
+14B |
| core/DateTimeInput | +30.2KB |
+15B |
| core/PowerSearch | +30.6KB |
+14B |
| core/TimeInput | +29.6KB |
+30B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | +21B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | 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`.
…he turn) into uikit/c21-wheel
…ps its mark after a nudge - A value changed from outside while the wheel is shown (a bound clamping the time after another wheel turned) turns the wheel onto its row, as UIDatePicker does and FR13 now says. The first park when the wheel is shown stays at once. - A motion that ends on the row it began on (a nudge that eases back, a drag off a row and back) gives the band's mark back to that row: the player forgets its last report when a motion starts, so the row it ends on is reported again. - The ctrl-wheel note cites FR43 (a ctrl-wheel is a pinch) and FR10 (with no pinch of its own, the wheel leaves it to the page's zoom).
…sheet on the desktop viewport This file's mouse cases run on a desktop viewport, where the field shows its popover unless it asks for the sheet; the case meta#8483 added now asks for it, as its neighbours do.
…e new foundation) into uikit/c21-wheel-proofs
… FR5 names The coast's proof now blocks the main thread for 1 s, the length spec:AST-094 FR5's verification names, ending while the coast is still over a row from rest so every frame moves a pixel or more. It follows the row that crosses the band halfway through, which stays in sight the whole second. The WebKit skip says what covers Mobile Safari instead: an iOS simulator capture through a 530 ms block.
…nto uikit/c21-wheel-listbox
The four Effects without a note now say what they synchronize: the latest props for callbacks attached once, the band's mark with the row the motion reports, the motion's end with the wheel's, and the latest touch-down handler.
…kit/c21-wheel-short-loop
…nd its seam - A rest on a disabled row measures the distance to the nearest enabled row around the loop, with a tie going toward the committed row, and turns the shorter signed way. It no longer walks the list as a straight line, which sent a rest beside the seam the long way round (a 4-row loop with :00 and :15 disabled, rested on :00 from :45, turned on to :30). - A key's search for the next enabled row stops half a turn from where it started, so it never turns the long way round onto the far side of a disabled stretch (up from January, with only January and February enabled, stays on January). - A short loop repaints the band's row when the options change, so a value whose enabled state changes under a resting wheel takes its new colour.
…nto uikit/c21-wheel-rest-and-spin
…les when the wheel goes away - spinTo turns only onto a row a rest would keep: a disabled row, an index outside the rows, or a row the owner's resolveRest moves off resolves false at once and nothing turns, on the touch wheel as on the scrolling column. Before, the touch wheel resolved true on the first arrival and then turned on to another row (spinTo(3) onto a disabled hour committed 9). - A turn still running when the wheel unmounts ends as stopped, so a pending spinTo resolves false instead of never settling. - spinTo takes no duration or curve of its own: every spin plays the tap spin, so no caller passes literal motion.
…es off, as the touch column does
The touch column's options lie under its drawn rows, so the proof taps the PM row's place on the screen, where the column's tap recognizer takes it, rather than asking for a tap on the option element.
…nto uikit/c21-wheel-proofs
…nto uikit/c21-wheel-listbox
…nto uikit/c21-wheel-short-loop
…nto uikit/c21-wheel-rest-and-spin
…o their own changesets In the same release meta#8179's time columns loop, so "no shipped column loops yet" would contradict it; the columns that loop say so themselves.
…heirs) into uikit/c21-wheel-rest-and-spin
…s own spin meta#8527's `spinTo` no longer takes a duration or a curve, so a column the time wheels move by themselves turns on the wheel's own spin: an animated `setContentOffset`, 0.3 s on Core Animation's ease-in-ease-out (`picker.tapSpin`, the role `timing.curves.move`), resolved under the column when the turn starts (spec:AST-094 FR4). TimeWheels no longer plays the literal `datePicker.wheels.columnTurn`, and the browser proof samples the AM/PM column's turn against that spin.
…against main, in the stepped wheel's own changeset No released minute wheel stopped at its first and last step: the sheet's minute wheel listed all 60 minutes, and the stepped wheel arrives with meta#8179, whose changesets now say that every column loops and that the minute wheel steps by `increment` or `timeIncrement`. This PR's own changeset, and its edit to meta#8179's, are removed; its test stays.
There was a problem hiding this comment.
Approval needs an engineering owner's decision first: component:TimeInput, current on main, still defines increment as the typed field's arrow-key step and the sheet's wheels as composed, then clamped, so it has to be updated if the touch sheet's minute wheel steps by increment / timeIncrement and AM/PM follows the hour, and the draft DateInput FR13 / DEC-1 / Owns and DateTimeInput FR8 text needs approval too. An engineering owner can record that decision by commenting /approve-spec followed by this pull request's full head SHA once the records say what was chosen, and the author may do so as an engineering owner. Whatever the decision, these need changes:
- In 12-hour time with
min="13:00", one ArrowUp on the hour wheel from 1:00 PM commits 7:00 PM instead of keeping 1:00 PM, because the key search inPickerWheel.tsxreaches exactly half a turn, where TimeWheels reads row 7 as PM. - The diff is 39,069 authored lines from the whole stack; once the predecessors land, the time-wheel change itself (about 2,495 lines) still needs to come under 2,000 lines or be split, for example by moving the minute step into its own pull request.
- The Chinese docs for
incrementandtimeIncrementstill say the sheet behaves the same on iPhone and Android, without real-device evidence; drop that, as the English text did, or add recordings.
[Automated review]
| // stops half a turn from the start (or at the target, when the key | ||
| // moves further): past it a row is nearer the other way round, so a | ||
| // key never turns the long way round onto it, as a rest never does. | ||
| const reach = Math.max(Math.abs(delta), Math.floor(count / 2)); |
There was a problem hiding this comment.
On a looping 12-row column this lets a key's onward search reach exactly half a turn. TimeWheels reads that row as the other half of the day (hourAfterTurn keeps the half at six rows), so in 12-hour time with min="13:00" one ArrowUp from 1:00 PM commits 7:00 PM, and by the same path ArrowDown from 11:00 PM under max="23:00" lands on 5:00 PM. Stopping the onward search short of half a turn (a tie stays on the committed row, as a rest does) keeps the time where it was; a 12-hour case beside the 24-hour one in TimeWheels.test.tsx would pin it.
| type: 'number', | ||
| description: | ||
| '用户按上或下方向键时增加或减少的分钟数。步长为整分钟；传入小数时时间保持不变。', | ||
| '用户按上或下方向键时增加或减少的分钟数。步长为整分钟；传入小数时时间保持不变。手指操作时（iPhone 与 Android 相同），底部面板的滚轮在该值能整除 60（1 到 30）时每隔 n 分钟显示一行，否则逐分钟显示，不在步长上的时间在滚轮转动前显示为最接近的一步；使用鼠标时滚轮逐分钟显示。', |
There was a problem hiding this comment.
This string still says the sheet behaves the same on iPhone and Android (“iPhone 与 Android 相同”), which the English text no longer claims. Without real-device recordings, please drop the parenthetical here and in DateTimeInput.doc.mjs.
Depends on #8178 and #8527. Review only this PR's change: https://github.com/vjeux/astryx/compare/06ed02d7be..92c5214db2
User need
Under a finger, the TimeInput and DateTimeInput sheet wheels differ from UIDatePicker's: 11 AM turned to 12 gives 12 AM, columns stop at the ends,
incrementis ignored, and pastmin/maxa time jumps. One touch model (spec:AST-094FR13); a mouse keeps today's.Evidence
Recorded on the iOS 26.2 simulator and the Android emulator (Chromium 158) from these heads; Overlays: JS clock, Reduce Motion, finger. No real device was available to the test rig. Before: #8178 + #8159 with these stories.
increment={15}, one row downmax5 PMBefore: 12 AM, stops at 12 and 59, 9:01, and a jump to 5. This PR: 12:58 PM with AM/PM following both ways, 58 to 00 keeping the hour, 9:15, and a turn onto 5 PM; Reduce Motion dips; blocked, AM/PM keeps turning.
Problem and solution fit
Under a finger the sheets render
TimeWheels(rules intimeWheelRules.ts, #8178's values, #8159's wheel):picker.tapSpinontiming.curves.move); under Reduce Motion, feat(Wheel): UIPickerView's motion under a finger, in today's look #8159's cross-fade.Current specification
spec:AST-043(current): wheels;spec:AST-094FR1, FR2: UIKit's values; bounds: #8131's decision (cixzhang, Oct 25).Public delta and caller burden
increment.analyze-pr.js), import gzip, base → head: TimeInput 117.9 → 119.7 KB, DateTimeInput 141.9 → 143.6 KB, DateInput unchanged, PowerSearch 196.9 → 198.8 KB; component CSS unchanged.Design and alternatives
Twelve hour rows (24 would read each label twice). AM/PM starts at the hour's rest, ~35 ms after UIKit's.
Scope
Testing
TimeWheels.test.tsxandtimeWheelRules.test.ts, failing before. In both sheets, under an iPhone and an Android finger, a controlled 8:00 under a 9:00 minimum stays 8:00 and an empty field stays empty.TimeWheels.test.tsxTimeWheels.test.tsx: AM/PM follows the hour both ways, the shorter way round; the columns loop and minutes don't carry; increments that divide 60 (≤ 30) with rounding; a time past a bound commits as the bound and its columns turn.TimeWheelsTouch.test.tsx: the same sheet under an iPhone and an Android finger (the Android case fails on the base, which kept today's columns); a mouse keeps today's columns.TimeWheels.interaction.browser.spec.ts, Chromium 7/7 and WebKit: the sampled turn; under Reduce Motion nothing travels, the AM/PM change being an opacity dip of 200 ms or less while its rows step onto their places at the dip's middle (the wheel's landing; the check fails without Reduce Motion); with a mouse, 11 to 12 stays AM.pnpm buildthen the rootpnpm typecheck;CI=true pnpm lint; the full core suite at 16 workers, 15,518 of 15,520 (the two Selector and MultiSelector bottom-sheet cases are test(Selector, MultiSelector): wait for the on-demand sheet module before the first open #8642's load red); the node project (its known reds, andtouch-support-bundle, which test(core): a touch component's import carries its gesture code, and nothing else does #8671 replaces: this head passes test(core): a touch component's import carries its gesture code, and nothing else does #8671's version, 8 of 8); interaction specs (MonthYearToggle, TimeWheels, MonthYearWheels, MonthScroller, MonthPager, WheelActivation) twice over: Chromium 93 of 94 (feat(Wheel): an owner's rest rule and turns from script for the picker wheel #8527's WheelActivation load red), WebKit 84 plus 10 skips.check:changesets.scripts/touch-support-bundle.test.mjs"is absent from the root entry" fails for every touch component under main's current test; test(core): a touch component's import carries its gesture code, and nothing else does #8671 replaces it with the per-component check, which lists DateInput, DateTimeInput, PowerSearch and TimeInput.Stack (merge in this order): … → #8156 → #8159 → #8178 → this PR (the full order is in #8156).