Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the draft component:DateInput FR13 and DEC-1 and component:DateTimeInput FR8, which the new resolveRest default relies on, are not on main, so an engineering owner needs to approve or reject them as written at this head by commenting /approve-spec <full head SHA> here (the author can do this as an engineering owner). Whatever that decision, three changes are needed before approval:
- Land #8366 and its predecessors first, then rebase so this pull request's diff against
mainis its one 296-line commit; today it carries 34,575 changed lines and conflicts withmain. - Make
spinToreport the same outcome under a finger and a mouse: under a finger it resolvestruefor a disabled row, for a rowresolveRestmoves off, and for an index past the list, where a mouse getsfalseor a different commit. - Settle a pending
spinTowhen the wheel unmounts mid-turn, for example by calling the running motion'sonEndfromWheelMotion.dispose().
[Automated review]
| if (outcome === 'arrived') { | ||
| arrive(end); | ||
| } | ||
| resolve(outcome === 'arrived'); |
There was a problem hiding this comment.
This resolves true as soon as the first turn arrives, even when arrive(end) then turns on to another row: spinTo(3) onto a disabled hour resolves true and commits 9, and with an even-hour resolveRest, spinTo(13) commits 14. An index past options is turned to off the strip's end (spinTo(30) on 24 rows leaves the band empty). TodayWheel refuses a disabled or missing row with false and ignores the rule, so the same call has a different outcome per pointer. One rule for both presentations (refuse, or resolve with whether the committed row is index) and cases for these three calls would close it.
| dispose(): void { | ||
| const motion = this.motion; | ||
| if (motion != null) { | ||
| const offset = this.offset(); | ||
| this.finish(motion); | ||
| this.restingOffset = offset; | ||
| this.render(offset, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
dispose() finishes the running motion without calling its onEnd, so a spinTo in flight when the wheel unmounts never settles (still pending 5 s after an unmount at 100 ms), although the handle promises false when a turn is interrupted. Calling motion.onEnd?.('stopped', offset) here, as stop() does, would settle it.
meta-cla
Bot
added
the
CLA Signed
# Conflicts: # packages/core/src/DateInput/wheelMotion.test.ts # packages/core/src/DateInput/wheelMotion.ts
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the draft component:DateInput FR13 and DEC-1 and component:DateTimeInput FR8, which the resolveRest default relies on, are not on main, so an engineering owner needs to approve or reject them as written at this head by commenting /approve-spec <full head SHA> here (the author can do this as an engineering owner). Whatever that decision, three changes are needed before approval:
- Land #8366 and its predecessors first, then rebase so this pull request's diff against
mainis its one 296-line commit; today it carries 34,325 authored lines and conflicts withmain. - Make
spinToreport the same outcome under a finger and a mouse: under a finger it resolvestruefor a disabled row, for a rowresolveRestmoves off, and for an index outsideoptions, where a mouse getsfalseor a different committed row. - Settle a pending
spinTowhen the wheel unmounts mid-turn, for example by calling the running motion'sonEndfromWheelMotion.dispose().
[Automated review]
The look's drum takes meta#8156's projection-aware hit test (rowExtent, and rowAtPoint answering the row drawn under a point through the perspective), keeping the look's own semantics: a position along the wheel, a copy at every position of a looping wheel. Its drum tests add meta#8156's. # Conflicts: # packages/core/src/DateInput/wheelDrum.ts
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the draft component:DateInput FR13 and DEC-1 and component:DateTimeInput FR8, which the resolveRest default relies on, are not on main, so approval waits on an engineering owner approving or rejecting them as written at this head by commenting /approve-spec <full head SHA> here (the author may do this as an engineering owner). Whatever that decision, three changes are needed before approval:
- Land #8366 and its predecessors first (or retarget onto its branch) so this pull request's diff against
mainis its one 296-line commit; today it carries 34,434 authored lines. - Make
spinToreport the same outcome under a finger and a mouse: under a finger it resolvestruefor a disabled row, for a rowresolveRestmoves off, and for an index outsideoptions, where a mouse getsfalseor commits a different row. - Settle a pending
spinTowhen the wheel unmounts mid-turn, for example by calling the running motion'sonEndfromWheelMotion.dispose().
[Automated review]
| if (outcome === 'arrived') { | ||
| arrive(end); | ||
| } | ||
| resolve(outcome === 'arrived'); |
There was a problem hiding this comment.
This resolves true as soon as the first turn arrives, even when arrive(end) has just started a turn on to another row: on hours 09-17 with 12 committed, spinTo(3) resolves true and then commits 9, while under a fine pointer TodayWheel answers false for the same call. Resolving after the final commit (or refusing rows the wheel cannot rest on) would give both pointers one answer.
| } | ||
|
|
||
| /** Release every resource; the rows stay where they are shown. */ | ||
| dispose(): void { |
There was a problem hiding this comment.
dispose() cancels the running motion without calling its onEnd, so a spinTo still turning when the sheet closes never settles. Calling motion.onEnd?.('stopped', offset) here, as stop() does, would resolve it false.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsDateInput (@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 | +28.4KB |
+15B |
| core/PowerSearch | +28.8KB |
+14B |
| core/TimeInput | +27.9KB |
+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
The look's drum takes meta#8156's projection-aware hit test (rowExtent, and rowAtPoint answering the row drawn under a point through the perspective), keeping the look's own semantics: a position along the wheel, a copy at every position of a looping wheel. Its drum tests add meta#8156's. # Conflicts: # packages/core/src/DateInput/__snapshots__/PickerWheel.test.tsx.snap
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the draft component:DateInput FR13 and DEC-1 and component:DateTimeInput FR8, which the resolveRest default relies on, are not on main, so approval waits on an engineering owner approving or rejecting them as written at this head by commenting /approve-spec <full head SHA> here (the author may do this as an engineering owner). Whatever that decision, four changes are needed before approval:
- Land #8366 and its predecessors first, reconciled with
main(where Lab now reaches core's touch modules through@astryxdesign/core/internal/touch), so this pull request's diff is its one 296-line commit instead of 36,108 authored lines. - Make
spinToreport the same outcome under a finger and a mouse: under a finger it resolvestruefor a disabled row, for a rowresolveRestmoves off, and for an index outsideoptions, where a mouse getsfalseor commits a different row. - Settle a pending
spinTowhen the wheel unmounts mid-turn, for example by calling the running motion'sonEndfromWheelMotion.dispose(). - Take the turn's
durationandcurveas motion roles (or drop the overrides), sincespec:AST-094FR4 forbids playing literal motion and the tap spin's 300 ms constant is not a role.
[Automated review]
| spin.duration ?? picker.tapSpin.durationMs / 1000, | ||
| spin.curve ?? resolveMotion(column, timing.curves.move), |
There was a problem hiding this comment.
spec:AST-094 FR4 says every duration and curve a touch presentation plays is a motion role and a component "MUST NOT play a literal". Here an owner's duration and curve are played as passed, and the fallback duration picker.tapSpin.durationMs is, per motion/uikit/picker.ts, "an internal constant, not a motion role". Taking a role resolved through resolveMotion (as the curve already is), or dropping the overrides until an owner needs one, would conform.
…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
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the draft component:DateInput FR13 and DEC-1 and component:DateTimeInput FR8, which the resolveRest default relies on, are not on main, so approval waits on an engineering owner approving or rejecting them as written at this head by commenting /approve-spec <full head SHA> here (the author may do this as an engineering owner). Whatever that decision, three changes are needed before approval:
- Land #8366 and its predecessors first, so this pull request's diff is its own 390-line change instead of 36,249 authored lines.
- Make
spinToresolve with whether the row it was asked for is the committed one: a re-render mid-turn that disables the target, or makesresolveRestmove off it, still resolvestruewhile another row commits. - Take the turn's duration from a registered motion role, since
spec:AST-094FR4 forbids playing a literal andpicker.tapSpin.durationMsis not a role.
[Automated review]
| if (outcome === 'arrived') { | ||
| arrive(end); | ||
| } | ||
| resolve(outcome === 'arrived'); |
There was a problem hiding this comment.
This resolves true right after arrive(end), which re-reads the latest options and resolveRest. If the owner re-renders mid-turn so index is now disabled, or its rule moves off it, the same row nodes come back, the turn goes on, arrive starts a second turn and another row commits, yet the promise has already said true (WheelHandle promises “true once that row is the committed one”). Resolving with whether arrive committed index without a further turn, with a test that disables the target mid-turn, would close it.
| return spinCurve( | ||
| from, | ||
| to, | ||
| picker.tapSpin.durationMs / 1000, |
There was a problem hiding this comment.
spinTo now plays this turn too, and picker.tapSpin.durationMs is, per motion/uikit/picker.ts, an internal constant that no table registers, so a theme's tempo never reaches it; spec:AST-094 FR4 asks every duration a touch presentation plays to be a motion role. Registering the tap spin's duration through the generated role table and resolving it here, as the curve already is, would conform.
| isDisabled?: boolean; | ||
| } | ||
|
|
||
| /** Options for a programmatic turn. */ |
There was a problem hiding this comment.
Nit: this doc comment is left over from the removed spin options and documents nothing now; the next comment is WheelHandle's.
…nd, the clamped turn) into uikit/c21-wheel-look The look draws the band's row through its copy, so the lower pull requests' painted-mark cases become rest cases here, and the clamped turn and the loop's rest play through the look's own play().
…nto uikit/c21-wheel-look
…heirs) into uikit/c21-wheel-look
|
The three code changes are in 1d7ee3e:
The compare still carries #8366 and the pull requests below it until they land; this pull request's own change is the rest hook and |
…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.
…he wheel's own spin meta#8527's `spinTo` no longer takes a duration or a curve: the month wheel turns onto the nearest reachable month on the wheel's own spin (`picker.tapSpin` on the role `timing.curves.move`, UIKit's animated `setContentOffset`), resolved when the turn starts (spec:AST-094 FR4), not on the literal `datePicker.wheels.columnTurn`.
…he nearest enabled month around the seam With min 2025-11-01, max 2026-02-28 and January 25, 2026 chosen, one ArrowUp from January crosses the seam onto December 2026, past max; around the loop the nearest enabled month is January itself, so nothing moves or commits. It went to February 25 before meta#8527's seam-aware rest and key step (f4386c5).
Depends on #8366. Review only this PR's change: vjeux/astryx@f3ce064...56a113f
User need
The time and date-time sheets' touch columns need two things the date wheels don't: a rest rule of their own (an AM/PM column follows the hour it belongs to) and turns from code (the hour wheel turning when AM/PM changes). Without them, those columns can't be built on the touch wheel. This adds both, for the pull requests that build those columns on this one.
Problem and solution fit
What led to it: Astryx's UIKit-parity work (
spec:AST-094). Both came with #8159, which uses neither, and review asked for them to arrive where an owner uses them, so they are split out here.resolveRest(index): the row a wheel moves on to after resting on another, then commits. By default, the nearest enabled row (fix(Wheel): a wheel that stops outside the bounds rests on the nearest value inside them #8131's rule).refwithspinTo(index): turns the wheel onto a row and commits it as a rest would; it reports false if a finger or another turn interrupts it. With a mouse, the scrolling column commits the row at once.component:DateInputFR13). Under Reduce Motion, a turn from code cross-fades, as a tap does.Current specification
spec:AST-094FR3, FR8, FR13;component:DateInputFR13;design:reduced-motionDR2.Public delta and caller burden
No API change:
Wheelis internal, and nothing in this stack passes either yet.Design and alternatives
spinTosays whether it arrived, and the owner never writes the offset.Scope
Testing
pnpm exec vitest run packages/core/src/DateInput/PickerWheel.test.tsx packages/core/src/DateInput/Wheel.test.tsx. The new cases pass: a spin lands and commits, a finger interrupting it reports false, a wheel rests where its owner's rule says, a spin survives the owner re-rendering mid-turn, Reduce Motion fades every turn, and with a mouse a spin commits at once.Merge order: #8366, then this PR, then #8480.