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🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch | 272.1 kB (+142 B) · 23.1 kB CSS (0) | — | — · — · — |
| DateInput · Storybook | 173.8 kB (+124 B) · 16.4 kB CSS (0) | 5,656 (+43) | ✓ · ✓ · ✓ |
| DateTimeInput · Storybook | 196.2 kB (+107 B) · 18.1 kB CSS (0) | 3,595 (+28) | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.79 MB packed (+1.3 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 24 rows; 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.
This review is on hold for a spec decision: whether the month and year wheels in the DateInput and DateTimeInput bottom sheets should change the chosen date, as iOS's inline date picker does, has no approved spec yet (component:DateInput and component:DateTimeInput are draft, and the pull request marks it as pending owner review), and the new draft spec:AST-094 would also amend the current spec:AST-025 FR14 and design:user-states DR1. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, these changes are needed before approval:
- Reduce this pull request's diff to its own commit: it currently carries the whole stack, about 36,700 changed lines, so land the predecessors first or base the branch on #8206.
- In DateTimeInput, the stepped time wheels round 23:59 to 12:00 AM without moving the date, so a turn of the minute wheel commits a time about 23 hours earlier on the same day.
spec:AST-094FR7 citesspec:AST-090, which does not exist in the repository, and the newdateConstraintsrefusal on the wheel path has no test.
[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.
In DateTimeInput this wrap drops a day: with timeIncrement={15}, a value of 2026-03-21T23:59 shows 12:00 AM, and turning the minute wheel to :45 commits 2026-03-21T00:45, because commitWheelTime keeps the date. Rounding down near midnight in the DateTimeInput sheet, or carrying the rounding into the date, avoids it; a test at 23:59 with a 15-minute step would pin it.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 does not exist at this head or on main, so the departure this requirement asks owners to accept cannot be checked. Drop the reference, or cite the record where it lives.
There was a problem hiding this comment.
This branch no longer edits the AST-094 record (#8378 on main carries it).
| min != null ? plainDateFromISO(min) : null, | ||
| max != null ? plainDateFromISO(max) : null, | ||
| ); | ||
| if (!plainDateIsEqual(moved, selectedDate) && !isDateDisabled(moved)) { |
There was a problem hiding this comment.
This refusal branch has no test in either sheet (TouchDateTimeField.tsx:834 too). A test where dateConstraints refuses the moved date, asserting that onChange is not called while the calendar still moves, would protect it.
There was a problem hiding this comment.
Added: both sheets test a refused date (May 21); head 0c6ff10.
ba61d2a to
e8f8953
Compare
There was a problem hiding this comment.
This review is on hold for a spec decision: whether the month and year wheels in the DateInput and DateTimeInput bottom sheets should change the chosen date, as iOS's inline date picker does, has no approved spec yet (component:DateInput and component:DateTimeInput are draft), and the new draft spec:AST-094 would also amend the current spec:AST-025 FR14 and design:user-states DR1. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, these changes are needed before approval:
- Reduce this pull request's diff to its own commit: it still carries the whole stack, about 36,500 changed lines, so land the predecessors first or base the branch on #8206.
- In DateTimeInput, the stepped time wheels round 23:59 to 12:00 AM without moving the date, so a turn of the minute wheel commits a time about 23 hours earlier on the same day.
spec:AST-094FR7 citesspec:AST-090, which does not exist in the repository, and the newdateConstraintsrefusal on the wheel path has no test.
[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.
In DateTimeInput this wrap drops a day: with timeIncrement={15}, a value of 2026-03-21T23:59 shows 12:00 AM, and turning the minute wheel to :45 commits 2026-03-21T00:45, because commitWheelTime keeps the date. Rounding down near midnight in the DateTimeInput sheet, or carrying the rounding into the date, avoids it; a test at 23:59 with a 15-minute step would pin it.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 does not exist at this head or on main, so the departure this requirement asks owners to accept cannot be checked. Drop the reference, or cite the record where it lives.
There was a problem hiding this comment.
This branch no longer edits the AST-094 record.
| min != null ? plainDateFromISO(min) : null, | ||
| max != null ? plainDateFromISO(max) : null, | ||
| ); | ||
| if (!plainDateIsEqual(moved, selectedDate) && !isDateDisabled(moved)) { |
There was a problem hiding this comment.
This refusal branch has no test in either sheet (TouchDateTimeField.tsx:834 too). A test where dateConstraints refuses the moved date, asserting that onChange is not called while the calendar still moves, would protect it.
There was a problem hiding this comment.
Added: both sheets test a refused date; head 0c6ff10.
|
|
||
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); |
There was a problem hiding this comment.
goTo reports synchronously through the caller's onIndexChange, so this Effect echoes a controlled move back to the owner, the Effect-driven callback the React runtime rules forbid, and its comment names no system it synchronizes. No caller passes index yet; skipping the report for a controlled move, or dropping the unused path, keeps it from biting later.
e8f8953 to
4f517fd
Compare
4f517fd to
a05d1b1
Compare
There was a problem hiding this comment.
This review is on hold for a spec decision: whether the month and year wheels in the DateInput and DateTimeInput bottom sheets should change the chosen date, as iOS's inline date picker does, has no approved spec yet (component:DateInput and component:DateTimeInput are draft), and draft spec:AST-094 in this range would also amend the current spec:AST-025 FR14 and design:user-states DR1. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, these changes are needed before approval:
- Reduce this pull request's diff to its own commit: it still carries the whole stack, about 37,200 changed lines, so land the predecessors first or base the branch on #8206.
- In DateTimeInput, the stepped time wheels round 23:59 to 12:00 AM without moving the date, so a turn of the minute wheel commits a time about 23 hours earlier on the same day.
spec:AST-094FR7 citesspec:AST-090, which does not exist in the repository, and thedateConstraintsrefusal on the new wheel path has no test in either sheet.
[Automated review]
| return hasSeconds ? time : {...time, second: 0}; | ||
| } | ||
| const minutes = time.hour * 60 + time.minute + time.second / 60; | ||
| const rounded = Math.round(minutes / interval) * interval; |
There was a problem hiding this comment.
In DateTimeInput this wrap drops a day: with timeIncrement={15}, a value of 2026-03-21T23:59 shows 12:00 AM, and turning the minute wheel to :45 commits 2026-03-21T00:45, because restOn turns from the shown time and commitWheelTime keeps the date (TouchDateTimeField.tsx:832-844). Rounding down at the end of the day, or carrying the rounding into the date, avoids it; a test at 23:59 with a 15-minute step would pin it.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 does not exist at this head or on main, so the departure this requirement asks owners to accept cannot be checked. Drop the reference, or cite the record where it lives once it lands (the current-state impact at lines 234-236 names it too).
| min != null ? plainDateFromISO(min) : null, | ||
| max != null ? plainDateFromISO(max) : null, | ||
| ); | ||
| if (!plainDateIsEqual(moved, selectedDate) && !isDateDisabled(moved)) { |
There was a problem hiding this comment.
This refusal branch has no test in either sheet (TouchDateTimeField.tsx:825 too). A test where dateConstraints refuses the moved date, asserting that onChange is not called while the calendar still moves, would cover the promise that a refused date never reaches the caller.
There was a problem hiding this comment.
Added in 278a61a: both sheets have a test where dateConstraints refuses the moved date (May 21). The date stays unchosen while the calendar moves.
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); | ||
| } | ||
| }, [pager, index]); |
There was a problem hiding this comment.
goTo reports synchronously through the caller's onIndexChange, so this Effect echoes a controlled move back to the owner, the Effect-driven callback the React component runtime contract rules out, and its comment names no external system. Naming the pager engine and skipping the report for a controlled move, or dropping the unused controlled path, would fix it.
a05d1b1 to
246ee17
Compare
There was a problem hiding this comment.
This pull request is on hold for a spec decision: no approved spec says whether the month and year wheels in the DateInput and DateTimeInput bottom sheets should change the chosen date (component:DateInput and component:DateTimeInput are draft), and the draft spec:AST-094 carried in this range would also amend current scrolling and press-state specs. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, these changes are needed before approval:
- Reduce the diff to this pull request's own commit: it still carries the whole stack, about 37,500 changed lines, so land the predecessors first or base the branch on #8206.
- In the DateTimeInput sheet, a stepped minute wheel rounds 23:59 to 12:00 AM without moving the date, so turning it commits a time about 23 hours earlier on the same day (
timeWheelRules.ts,TouchDateTimeField.tsx). spec:AST-094FR7 citesspec:AST-090, which does not exist; thedateConstraintsrefusal on the new wheel path has no test in either sheet; andusePager's controlled-index Effect callsonIndexChangefrom an Effect.
[Automated review]
246ee17 to
278a61a
Compare
d3bc5f0 to
4832d6e
Compare
|
Owner decision. The spec owners ruled on both touch behaviors (Nov 2):
#8159 records both in Size. This pull request's own change is 580 lines (vjeux/astryx@ed7552d...4832d6e). It stacks on #8179 in the B4 order and rebases onto |
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision on the wheel records, and it needs changes either way.
Owner decision needed: component:DateInput FR17 steps a 12-hour hour wheel in time order (at a 12:00–17:00 bound, ArrowDown from 5 PM commits nothing and from 12 PM commits 1 PM), while the Nov 2 ruling quoted on the pull request says a key moves to "the next enabled row in the key's direction", which from 5 PM is the 12 row. DEC-2's flat rows under a finger, component:TimeInput FR7 and the other record edits were approved only at an earlier head. An engineering owner records the decision by commenting /approve-spec <full head SHA> or by amending the records on main; the author may do so as an engineering owner.
Required changes:
- The range against
mainis 10,674 authored lines around this pull request's own 610-line change, over the owner-directed batch ceiling of 8,000, and it conflicts withmaininmotion-roles.mdandmotion/uikit/index.ts, so land the stacked wheel commits first and rebase onto the two own commits. snapCurveinwheelPhysics.tsplayspicker.snapRate, a literal rather than a motion role, againstspec:AST-094FR4, so register the snap as a role or have FR4 record it as a gap, as it does for the pager.- The 12-hour conversion now lives in three copies (
hour12Ofand the inline 12-to-24 mapping intimeWheelRules.ts, andhour12From24/hour24From12inTouchTimeField.tsxandTouchDateTimeField.tsx), so export one pair fromtimeWheelRules.tsand call it from both sheets.
The month/year change itself works as described: on a phone, a tap on February moves January 25 to February 25, and a mouse still moves only the calendar.
[Automated review]
| */ | ||
| export function snapCurve(from: number, to: number, scale = 1): WheelCurve { | ||
| const distance = from - to; | ||
| const rate = picker.snapRate * 1000; |
There was a problem hiding this comment.
picker.snapRate is a literal, not a motion role: spec:AST-094 FR4 says a component "MUST read touch motion through these roles and MUST NOT play a literal", so a theme's motion knobs never reach this snap. Register it as a role, or have FR4 name the picker snap in its implementation gaps, as it does for the pager.
| } | ||
|
|
||
| /** The 12-hour clock's label for a 24-hour hour: 0 → 12, 13 → 1. */ | ||
| export function hour12Of(hour: number): number { |
There was a problem hiding this comment.
hour12Of is the same conversion as hour12From24 in TouchTimeField.tsx:153 and TouchDateTimeField.tsx:420, and :228 and :306 repeat their hour24From12. Export one pair from here and call it from both sheets' composeWheelTime, so a fix reaches the mouse columns too.
| 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 the scrolling column by a date. Suggested: "The scrolling column (Wheel.tsx): the same box, band and rows, moved by the gesture system and the compositor." The same applies at :95 and useWheelListbox.ts:44.
|
/approve-spec 0a1769a |
|
This head. Rebased onto Size (INV6), main cae1f66 → this head: DateInput 103.0 → 144.3 KB, DateTimeInput 114.9 → 156.4 KB, PowerSearch 183.6 → 225.1 KB, MobilePowerSearch 168.5 → 209.7 KB, TimeInput 90.1 → 130.5 KB, almost all of it #8159's shared touch runtime. This pull request's own part, #8179 → this head: +0.1 to +0.2 KB for DateInput, DateTimeInput, PowerSearch and MobilePowerSearch; CSS unchanged. Lines. This pull request's own change is 580 authored lines, 462 of them tests (vjeux/astryx@986b0bb...0a1769a). It merges after #8159, #8178 and #8179 in B4's order. |
There was a problem hiding this comment.
Changes requested. The month/year wheel behaviour itself is owner-approved and its tests pass; two things need fixing before this can merge.
- This range carries 10,037 authored lines from 17 commits around this pull request's own 610-line change; please land #8159 and #8179 first and rebase so this pull request contains only its two commits.
- Two changesets present behaviour that already shipped as new:
.changeset/wheel-uikit-picker.mdsays a value changed from outside "turns the wheel onto its row instead of jumping there" (the shown wheel already turned onto such a value), and the second bullet of.changeset/time-wheels-follow-uidatepicker.mdlists the rest pastmin/max, the other columns turning, and opening never changing the value, all of which already worked; please reword them to describe only what changes.
Non-blocking: .changeset/wheel-interaction-proofs.md calls the mouse column "today's column"; "the scrolling column" reads better in release notes.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Under a coarse primary pointer, as on a phone or a tablet, the picker wheels in the TimeInput, DateTimeInput and DateInput bottom sheets now move like UIPickerView. Their rows keep the 28 px pitch and lie flat, fading toward the edges of the column in every browser; the scrolling column under a mouse tilts and fades its rows only where the browser supports scroll-driven animations, and shows a plain list elsewhere. The row under the band is in semibold accent text on its plate while the wheel rests and while a finger drags it; no row is marked while the wheel coasts, snaps or turns. A flick coasts at UIKit's deceleration onto the row UIKit's projection picks. A finger landing on a moving wheel stops it. A tap on a row turns it onto the band, and a value changed from outside while the sheet is open, as when a bound clamps the time, turns the wheel onto its row instead of jumping there. Past the first or last row, a drag rubber-bands and the wheel bounces back. As on iOS, the value commits when the wheel comes to rest, and the coast keeps running while the page's main thread is busy. Arrow keys and clicks on a row still commit at once. Under reduced motion a flick still coasts, as scrolling does; a tap, a key or a slow release cross-fades onto its row. `presentation="popover"` keeps the previous surface; no value keeps the native-scroll column under a finger. Each row that reaches the band ticks with a short vibration where the browser supports it, as on Android; iOS Safari has no vibration. Under a fine primary pointer, as with a mouse on a desktop, the wheels keep the scrolling column. |
There was a problem hiding this comment.
A shown wheel already turns onto a value changed from outside on main (Wheel.tsx's park effect smooth-scrolls to it, as component:DateInput FR14 requires). Please drop "instead of jumping there" so the release note does not announce a fix for something that worked.
| [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.
Everything in this bullet already worked under a finger on main: the rest goes on to the nearest enabled row, the composed time is clamped into the bounds, the other columns turn onto it (FR14), and opening commits nothing (FR13). Please keep only the AM/PM bullet as the change, or mark this one "as before".
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on this head's spec edits, and the size of the range needs fixing either way.
Owner decision needed: the edits to component:DateInput, component:DateTimeInput, component:TimeInput and spec:AST-094 were approved only at the previous head, and a new commit cancels that approval. This head also narrows DateInput FR14's tap-spin sentence and FR18 to "Under a finger", and changes what the exported clampTime returns for a bound between whole minutes, which no record covers for direct callers. An engineering owner records the decision by commenting /approve-spec <full head SHA> on the pull request (the author may do so as an engineering owner), or by amending the records.
Blocking:
- The pull request carries 10,109 authored lines, above the 8,000-line batch ceiling, around its own 612-line change; land #8159, #8178 and #8179, then rebase so this pull request is its own two commits.
The two changesets flagged last time now describe only what changes. Four new comments restate the code under them; see the inline notes (non-blocking).
[Automated review]
| (next: number) => { | ||
| setMonthIndex(next); | ||
| scrollerHandleRef.current?.scrollToMonth(next, 'auto'); | ||
| // Only the finger's wheels move the date; the scrolling ones move the calendar. |
There was a problem hiding this comment.
Non-blocking: the block comment above already says a mouse moves only the calendar, and this line restates the if below it. Suggest dropping it.
| (next: number) => { | ||
| setMonthIndex(next); | ||
| scrollerHandleRef.current?.scrollToMonth(next, 'auto'); | ||
| // Only the finger's wheels move the date; the scrolling ones move the calendar. |
There was a problem hiding this comment.
Non-blocking: same as in TouchDateField.tsx; the block comment above covers it. Suggest dropping this line.
| ); | ||
| 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: this restates the isTouch ternary below. Suggest dropping it, or keeping only why the two pointers get different wheels.
| 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 restates the isTouch ternary below. Suggest dropping it, or keeping only why the two pointers get different wheels.
|
Size (INV6), main cae1f66 → c5f6df1, gzip. JavaScript: DateInput 103.0 → 144.3 KB, DateTimeInput 114.9 → 156.4 KB, PowerSearch 183.6 → 225.1 KB, MobilePowerSearch 168.5 → 209.6 KB, TimeInput 90.1 → 130.5 KB. CSS: TimeInput +73 B, DateInput +51 B, DateTimeInput +49 B, PowerSearch +47 B, MobilePowerSearch +0 B, all from #8159's wheel styles. Almost all of it is #8159's shared touch runtime, which a page pays once (spec owners, Nov 2). This pull request's own part, #8179 → this head: +0.1 to +0.2 KB for DateInput, DateTimeInput, PowerSearch and MobilePowerSearch; CSS unchanged. Lines. After #8159, #8178 and #8179 land, this pull request is its own two commits. |
|
/approve-spec d060a1c |
There was a problem hiding this comment.
Changes requested. The month and year wheel behaviour and every record edit are approved by an engineering owner at this head, and the unit tests pass; the range has to shrink before it can merge.
- This range carries 10,104 authored lines from 18 commits around this pull request's own 610-line change, above the 8,000-line limit for an owner-directed batch; the 16 predecessor commits already have their own pull requests (#8159, #8178, #8179), so land those and rebase onto
mainso this pull request is its own two commits.
Non-blocking: four comments in PickerWheel.tsx and Wheel.tsx restate the line below them (inline notes).
[Automated review]
| [motion], | ||
| ); | ||
|
|
||
| // The motion's layout follows the rows. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates motion.setLayout(layout) below. Suggest dropping it.
| } | ||
| }, [isActive, motion]); | ||
|
|
||
| // A finger lands: a moving wheel stops where it is on screen. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates what onTouchDown does. Suggest dropping it.
| return () => column.removeEventListener('wheel', onWheel); | ||
| }, [isActive, moveBy]); | ||
|
|
||
| // Home and End: the first and the last enabled row. |
There was a problem hiding this comment.
Non-blocking (INV29): moveToEnd and endIndex already say this. Suggest dropping it.
| [moveTo, options, activeIndex], | ||
| ); | ||
|
|
||
| // Home and End: the first and the last enabled row. |
There was a problem hiding this comment.
Non-blocking (INV29): same as in PickerWheel.tsx; toEnd and endIndex already say this.
|
/approve-spec 2331365 |
There was a problem hiding this comment.
Changes requested. The month and year wheel behaviour and every record edit are approved by an engineering owner at this head, and the affected unit tests and the core typecheck pass; the range has to shrink before it can merge.
- This range carries 10,145 authored lines from 23 commits around this pull request's own 645-line change, above the 8,000-line limit for an owner-directed batch; the 21 predecessor commits already have their own pull requests (#8159, #8178, #8179), so land those and rebase onto
mainso this pull request is its own two commits.
Non-blocking: four comments restate the line below them (PickerWheel.tsx:361, :443, :687 and Wheel.tsx:540); dropping them keeps the source to its reasons.
[Automated review]
|
/approve-spec 5c5f34d |
There was a problem hiding this comment.
Changes requested. An engineering owner approved every record edit at this head, and the date move, the AM/PM rule and the clampTime fix each have passing tests; one scope item blocks.
- The diff against
mainis 3,836 authored lines around this pull request's own 590-line change, because it also carries #8178 and #8179, and no size exemption applies: the stack exemption needs the sameStack:line in every listed pull request, but #8200 and #8206 list #8159 first, and the batch exemption covers only changes a current spec already settled, which FR19 and the other record edits were not; make theStack:line identical in #8178, #8179, #8209, #8200 and #8206, or land #8178 and #8179 first so this pull request is its own commit.
Non-blocking: the comments above handleWheelChange in TouchDateField.tsx and TouchDateTimeField.tsx restate the handler; keep only the reason (see the inline note).
[Automated review]
| // Turning the month or year wheel moves the calendar. Under a finger it | ||
| // also moves a chosen date with it, keeping its day (UIDatePicker's inline | ||
| // calendar: datePicker.calendar.monthYearWheels.changesDate), clamped to | ||
| // the month and the bounds. A date a custom constraint refuses stays. |
There was a problem hiding this comment.
Non-blocking (comment style): these lines restate what handleWheelChange does step by step. Keep only the reason, for example:
// Under a finger, UIDatePicker's month and year wheels also move the chosen date
// (datePicker.calendar.monthYearWheels.changesDate).The copy above handleWheelChange in TouchDateTimeField.tsx can take the same line.
|
/approve-spec 038a612 |
There was a problem hiding this comment.
Changes requested. Every record edit (DateInput FR20, DateTimeInput FR11, TimeInput FR7) is approved by an engineering owner at this head, and the month/year date move, the AM/PM rule and the whole-minute clamp each have passing unit tests; one scope item blocks.
- The diff against
mainis 3,748 authored lines around this pull request's own 615-line change because it also carries #8179's three commits, and no size exemption applies, since the stack exemption needs one identicalStack:line in every listed pull request while this one reads#8179 → #8209 → #8200 → #8206 (#8178 merged)and #8179, #8200 and #8206 still list #8178 first: make the line identical in all four, or land #8179 first so this pull request is its own commit.
Non-blocking: the comments above handleWheelChange in TouchDateField.tsx and TouchDateTimeField.tsx restate the handler, so keep only the reason (see the inline notes). No record says whether a date the wheels move is announced to a screen reader; a sentence in DateInput FR20 would settle it.
[Automated review]
| // Turning the month or year wheel moves the calendar. Under a finger it | ||
| // also moves a chosen date with it, keeping its day (UIDatePicker's inline | ||
| // calendar: datePicker.calendar.monthYearWheels.changesDate), clamped to | ||
| // the month and the bounds. A date a custom constraint refuses stays. |
There was a problem hiding this comment.
Non-blocking: this comment restates what handleWheelChange does, and it runs on from the scroller comment above it. Keep only the reason, for example:
// UIDatePicker moves a chosen date with these wheels
// (datePicker.calendar.monthYearWheels.changesDate).| // Turning the month or year wheel moves the calendar. Under a finger it also | ||
| // moves a chosen date with it, keeping its day and the time, as | ||
| // UIDatePicker's inline calendar does | ||
| // (`datePicker.calendar.monthYearWheels.changesDate`). The day clamps to the | ||
| // month and the bounds. A date a custom constraint refuses stays. |
There was a problem hiding this comment.
Non-blocking: same as in TouchDateField.tsx; the reason is the UIDatePicker rule, and the rest restates the handler.
|
/approve-spec aae9f16 |
There was a problem hiding this comment.
Changes requested. An engineering owner approved the new DateInput FR20 and DateTimeInput FR11 rows at this head, and the date move under a finger has passing unit tests; one shared-code item blocks.
- Miss:
dateInMonthinpackages/core/src/DateInput/monthGeometry.ts(lines 109–114) repeats the day clamp ofplainDateAddMonthsinpackages/core/src/utils/plainDate.ts, which Calendar already calls for the same keep-the-day rule, so build the moved date withplainDateAddMonths(date, monthIndex - monthIndexOf(date))and keep only themin/maxclamp.
Non-blocking: the comments above handleWheelChange in both sheets restate the handler (see the earlier inline notes), and DateInput's Compatibility row and class do not yet mention FR20's new commit path. The Stack: line here differs from the one in #8200 and #8206, which still list #8179, so this pull request was reviewed on its own.
[Automated review]
| const {year, month} = fromMonthIndex(monthIndex); | ||
| const moved = { | ||
| year, | ||
| month, | ||
| day: Math.min(date.day, getDaysInMonth(year, month)), | ||
| }; |
There was a problem hiding this comment.
Miss: these lines repeat plainDateAddMonths in utils/plainDate.ts (the same month mapping and Math.min(day, getDaysInMonth(year, month)) clamp), which Calendar's PageUp and PageDown already call for the same keep-the-day rule. Calling it keeps one copy of the month arithmetic:
const moved = plainDateAddMonths(date, monthIndex - monthIndexOf(date));
|
/approve-spec f937fb6 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet; it starts once the pull request leaves draft and a maintainer adds the ci:run label.
An engineering owner approved DateInput FR20 and DateTimeInput FR11 at this head, and dateInMonth now reuses plainDateAddMonths, which resolves the earlier blocking item.
- Non-blocking: DateInput's Compatibility row and class do not yet mention FR20's new commit path.
- The
Stack:line here differs from the one in #8200 and #8206, which still list #8179, so this pull request was reviewed on its own.
[Automated review]
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8206.
…ove the date like UIDatePicker's Under a finger, turning the month or year wheel behind the sheet calendar's title moves a chosen date to the wheels' month and keeps its day: October 15 becomes May 15, and a day the month lacks becomes its last day (January 31 becomes February 28), inside min and max, by the months of the calendar the sheet shows; DateTimeInput keeps the time. A date dateConstraints refuses is not chosen, and with no date chosen the wheels move only the calendar. With a mouse the wheels move the calendar only, as today. The month wheel does not loop. DateInput FR22 records the rule under a coarse primary pointer, and DateTimeInput FR14 does the same and keeps the time. Each has a verification row.
…n end, not on still frames before a deadline
`openWheels` waited for the sheet, and then for the wheels, with the
harness's `waitForRest`: three still frames within 5 s. On a loaded
runner the page can present no frame for 6 s right after the sheet
opens, so the wait ran out with nothing moving ("still moving", while
the page held no animation). The proof now checks the sheet's open
transform, and then the wheels' full opacity, once a frame with no
deadline of its own, and waits on each animation's `finished` promise.
A starved runner only makes it later.
The comments above both sheets' handleWheelChange keep only the reason: UIDatePicker moves a chosen date with the wheels. The day and bound clamps read from dateInMonth itself, which moves the date by the calendar's own months through calendarShiftMonths.
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8206.
|
/approve-spec a98a089 |
Main gained meta#10285, which adds a `getOnlyButton` import beside the testing-library import this pull request widens in DateTimeInputTouch.test.tsx. The merge keeps both imports; this pull request's own changes are unchanged.
|
/approve-spec 6c7cecb |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8200.
Batch: B4 · Wheel / picker (DateInput, TimeInput, DateTimeInput)
Stack: #8209 → #8200
This pull request is three commits on
main9c80ad1. In the Gregorian calendar its product diff matches d060a1c's, where the phone check passed.User impact
Under a finger, turning the month or year wheel behind the DateInput or DateTimeInput sheet calendar's title moves only the calendar. A chosen date stays in its old month, where UIDatePicker moves the date with the wheels.
Problem and solution fit
dateInMonth). A day the month lacks becomes its last day (January 31 becomes February 28), and the date stays withinminandmax. DateTimeInput keeps the time.dateConstraintsrefuses is not chosen; with no date chosen, the wheels move only the calendar. Opening the wheels changes nothing.Expected behavior and authority
component:DateInputFR22 andcomponent:DateTimeInputFR14, as recorded here, with a verification row each. Each cites DateInput FR14's turn-onto rule./approve-specat this head.family:input-fieldsFR6 foronChange, thenchangeAction, and the optimistic value.Smallest restoration
TouchDateField.tsxandTouchDateTimeField.tsx(the wheels' change handler),monthGeometry.ts(dateInMonth), and the two records' rows.Writing rubric results
Over DateInput FR22, DateTimeInput FR14 and their verification rows (prettier re-pads DateInput's design table): R1 narration, R2 research, R4 rejected alternatives, R5 change references, R6 open questions: none. R3 evidence: test names only.
Scope
Testing
This head, a98a089, has three commits on
main9c80ad1. They are the earlier head f937fb6's three, rebased past #9643/#9842 (see "Changed by the rebase" below). The first moves the date by the active calendar's months. The second, the wheels' spec wait, is unchanged. The third keeps only its comment trims, because the month move now goes throughcalendarShiftMonths.pnpm install --frozen-lockfile0;check:knowledge0;check:changesets0;pnpm -F @astryxdesign/core typecheck0;CI=true pnpm lint0 errors; the DateInput, TimeInput, DateTimeInput, Calendar, utils and hooks unit tests: 3,472 passed, including the Persian and Hebrew month-move case.MonthYearWheelsandTimeWheelsinteraction specs against Storybook built at this head: Chromium 8 passed; WebKit (--cpus=4,LP_NUM_THREADS=4), twice: 14 passed, 2 skipped (the blocked-main-thread case, by design).pnpm review:pre0.Evidence
MonthYearWheels.interaction.browser.spec.ts"a turn of the month wheel moves the chosen date, keeping its day", under a real touch.DateInputTouch.test.tsx"under a finger: moves the chosen date with the wheels, keeping its day" andDateTimeInputTouch.test.tsx"under a finger: moves the chosen date with the month wheel, keeping the day and the time".Changed by the rebase onto #9643/#9842
Main now shows the sheet in the active calendar (#9643/#9842), so this head moves the date by that calendar's months.
dateInMonth(model, date, monthIndex, min, max)callscalendarShiftMonthsfrom the month model, where it called GregorianplainDateAddMonths, and both sheets pass theirmonthModel. In a Gregorian-aligned calendar the result is byte-for-byte the same.DateInputTouch.calendar.test.tsx"moves a date by calendar months, keeping its day of the month": Persian 15 and 31 Shahrivar become 15 and 30 Mehr; Hebrew 30 Adar I 5784 becomes 29 Adar 5785.