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 | +45.1KB |
+11B |
| core/DateTimeInput | +47.6KB |
+10B |
| core/PowerSearch | +47.0KB |
+4B |
| core/TimeInput | +29.6KB |
+19B |
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 | +22B |
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.
This review waits on an engineering owner: one month per release with the 45° sheet split, the reworded draft PR2 in DateInput.spec.md and the new draft spec:AST-094 have no current spec authority, so an engineering owner needs to approve or reject them by commenting /approve-spec 39408932bf15c4b71cf42bb94ee91e9fe674496f on this pull request, which the author may do as an engineering owner. Whatever that decision, please also make these changes:
- Land the predecessor pull requests, or rebase onto them, so this one carries only its own commit.
- Keep the
usePagereffects at lines 681-683 and 698-702 from callingonIndexChange, and name the system each synchronizes. - Correct the changeset's 45° line: drags between about 26.6° and 45° move neither the sheet nor the calendar.
[Automated review]
| pager.pan.setEnabled(isPanEnabled); | ||
| }, [pager, isPanEnabled]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This effect (and the setOptions effect at lines 681-683) can call the owner's onIndexChange through goTo(..., {reason: 'controlled'}), and neither names the system it synchronizes. Current architecture:react-component-runtime INV2 keeps consumer callbacks out of effects and INV3 asks each effect to name its external system: name the pager here and keep a 'controlled' move from calling onIndexChange, or drop the unused controlled index option.
| - A swipe moves at most one month, chosen at the lift by iOS's paging rule (a quick flick, or past half a month). It settles on iOS's paging curve instead of letting a fling run across several months. | ||
| - The month title follows the drag. It changes when the next month is more than half in view. | ||
| - The arrows scroll the month in 0.3 s; a second tap mid-scroll goes on to the month after. | ||
| - Inside the sheet, a drag steeper than 45° moves the sheet, and a flatter one stays with the calendar. |
There was a problem hiding this comment.
Drags between about 26.6° and 45° move neither the sheet nor the calendar (MonthScroller.tsx lines 457-464), so "a flatter one stays with the calendar" promises a response that does not happen. Suggest: "a drag steeper than 45° moves the sheet, one flatter than about 27° pages the calendar, and one in between moves neither, as on iOS."
There was a problem hiding this comment.
The changeset now says it: a drag steeper than 45° moves the sheet, one flatter than about 27° pages the calendar, and one in between moves neither, as on iOS.
| } | ||
| } else if (state === 'ended' || state === 'cancelled') { | ||
| // The release has chosen its month by now. | ||
| report(pager.virtualIndex); |
There was a problem hiding this comment.
This reports the release month synchronously at the end of the pan, so when a flick pages before the drag crossed half a month the owner's render runs inside the lift handler. usePager.ts lines 307-314 defer the pager's own report by two frames for exactly that Mobile Safari case; reporting from the deferred onIndexChange('drag') (lines 366-375) would keep the two consistent.
There was a problem hiding this comment.
Fixed in 68b7c0c: the title changes at half a month, not at the lift (see the newer thread).
| // 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 ("Typed-field only ... neither reaches this surface") no longer holds: increment now steps the sheet's minute wheel.
| hourFormat = '12h', | ||
| // Desktop-only: mobile uses wheels instead of typed arrow stepping or a preset list. | ||
| timeIncrement: _timeIncrement, | ||
| timeIncrement, |
There was a problem hiding this comment.
The comment above ("Desktop-only: mobile uses wheels instead of typed arrow stepping") no longer holds: timeIncrement now steps the sheet's minute wheel.
3940893 to
2631091
Compare
There was a problem hiding this comment.
This review waits on an engineering owner: one month per release with the 45° sheet split, the reworded draft PR2 in DateInput.spec.md and the new draft spec:AST-094 have no current spec authority, so an engineering owner needs to approve or reject them by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner. Whatever that decision, these changes are still needed:
- Land the predecessor pull requests or rebase onto them, so this one carries only its own commit.
- Keep the
usePagereffects at lines 681-683 and 698-702 from callingonIndexChange, and name the system each synchronizes. - Stop naming the release month at the lift in
MonthScroller.tsx(lines 418-421), which turns the title earlier than the measured UIKit behavior and the changeset describe. - Page on a horizontal trackpad or wheel gesture again (
MonthScroller.tsxline 106), and correct the changeset's 45° line, the two staleincrement/timeIncrementcomments and the citation of a missing AST-090 inspec:AST-094.
[Automated review]
| } | ||
| } else if (state === 'ended' || state === 'cancelled') { | ||
| // The release has chosen its month by now. | ||
| report(pager.virtualIndex); |
There was a problem hiding this comment.
This names the release month at the lift, so a flick released before half a month turns the title then. The head's own UIKit table says the title changes "when the content crosses half a page, during the drag (and during a settle), not at the lift" (motion/uikit/datePicker.ts lines 64-67), and the changeset says it changes "when the next month is more than half in view". Letting the pager's deferred onIndexChange('drag') (lines 366-375) report the release would also keep the owner's render out of the touch handler, as usePager.ts lines 307-314 intend.
There was a problem hiding this comment.
Fixed in 68b7c0c: under a finger the calendar is MonthPager. Its title follows the drag and the settle and changes at half a month, not at the lift (interaction spec: the title at half a month).
| // nothing to fight over, and a downward drag on the calendar can go back | ||
| // to meaning swipe-to-dismiss. | ||
| touchAction: 'pan-x', | ||
| overflow: 'hidden', |
There was a problem hiding this comment.
With the viewport overflow: 'hidden' and no wheel handling in the pager or the gesture host, a two-finger trackpad swipe or Shift+wheel over the calendar no longer pages; on the base it scrolled the months natively. Mapping a horizontal wheel gesture onto pager.goTo one month at a time would keep that path.
There was a problem hiding this comment.
Fixed in 68b7c0c: MonthPager pages one month per horizontal trackpad swipe or Shift+wheel gesture (interaction spec).
| 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.
Neither main nor this branch has an AST-090 record, so this amendment (and the one at line 235) points at text a reader cannot open; cite it once the record exists, or drop it.
2631091 to
161f142
Compare
There was a problem hiding this comment.
This review waits on an engineering owner: one month per release with the 45° sheet split, the reworded draft PR2 in DateInput.spec.md and the new draft spec:AST-094 have no current spec authority, so an engineering owner needs to approve or reject them by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner. Whatever that decision, these changes are still needed:
- Land the predecessor pull requests or rebase onto them, so this one carries only its own commit.
- Keep the
usePagereffects at lines 681-683 and 698-702 from callingonIndexChange, and name the system each synchronizes. - Stop naming the release month at the lift in
MonthScroller.tsx(lines 418-421), or document it, so the title, the changeset and the measured UIKit behavior agree. - Page on a horizontal trackpad or wheel gesture again (
MonthScroller.tsxline 106), and correct the changeset's 45° line, the two staleincrement/timeIncrementcomments and the citation of a missing AST-090 inspec:AST-094.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
161f142 to
68b7c0c
Compare
…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.
There was a problem hiding this comment.
Changes are needed before this can be approved: the one-month paging direction is settled under spec:AST-094, and what remains is scope, one carried spec contradiction, real-device evidence and a one-line comment.
- Land the predecessor pull requests (#8179, #8146, #8496 and the stacks under them), or rebase onto them, so this branch carries only its own 14 files; today it is about 46,700 authored lines, about 2,400 of them its own.
- The carried time-wheel change from #8179 makes TimeInput's
incrementstep the sheet's minute wheel under a finger, which the current TimeInput spec does not allow, so it needs settling in #8179. - Add real iPhone (Safari) and Android phone (Chrome) recordings of this head, since
spec:AST-094requires real-device evidence for touch behavior and the overshoot being fixed is an iOS Safari behavior. - Name what the pending-row layout Effect in
MonthPager.tsxsynchronizes, as the React runtime architecture's INV3 asks of every new Effect.
[Automated review]
| // Deliberately un-keyed: it runs after every render, costs a null check, and | ||
| // must fire on whichever render finally mounts the pending row's pane. | ||
| useLayoutEffect(() => { | ||
| const row = pendingRowRef.current; | ||
| if (row == null) { | ||
| return; | ||
| } | ||
| pendingRowRef.current = null; | ||
| pager.goTo(row, {animated: false}); | ||
| }); |
There was a problem hiding this comment.
This Effect calls pager.goTo(row, {animated: false}), and with the default reason usePager reports synchronously, so onIndexChange runs setCenterRow inside the Effect. The comment says when it fires but not what it synchronizes; architecture:react-component-runtime INV3 asks every new Effect to name its external system. Naming the pager's track position, which waits for the pending pane to mount, would cover it.
| time={parsedWheelTime} | ||
| hourFormat={hourFormat} | ||
| hasSeconds={hasSeconds} | ||
| minuteInterval={minuteIntervalOf(increment)} |
There was a problem hiding this comment.
Carried from #8179: under a finger this makes increment step the sheet's minute wheel, while the current TimeInput spec scopes increment to "Arrow-key step." on the typed surface. It needs settling in #8179 (an engineering owner amends component:TimeInput, or the sheet keeps listing every minute) before it reaches this branch.
…names what it synchronizes The un-keyed layout effect that lands a far jump synchronizes the pager's position with the DOM: it can move only once the render that mounts the target's pane has committed it, and moves before paint (architecture:react-component-runtime INV3).
There was a problem hiding this comment.
Changes are needed before this can be approved: the one-month paging direction is settled under spec:AST-094, and the latest push resolves the Effect comment, so what remains is scope, one carried spec contradiction and real-device evidence.
- Land the predecessor pull requests (#8179, #8146, #8496 and the stacks under them), or rebase onto them, so this branch carries only its own 14 files; today it is about 46,700 authored lines, and its own change is about 2,400 of them.
- The carried time-wheel change from #8179 makes TimeInput's
incrementstep the sheet's minute wheel under a finger, which the current TimeInput spec does not allow, so it needs settling in #8179. - Add real iPhone (Safari) and Android phone (Chrome) recordings of this head, since
spec:AST-094requires real-device evidence for touch behavior and the overshoot being fixed is an iOS Safari behavior.
[Automated review]
|
Depends on #8179, #8146 and #8496. Review only this PR's change: https://github.com/vjeux/astryx/compare/3478da6206..693fafc0c2
User need
Under a finger, the sheets' calendar is a native scroll-snap scroller: on iOS 26.2 a 300 pt flick can reach a second month. One touch model (
spec:AST-094FR13); a mouse keeps the scroller.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: #8179 + #8146 + #8496 with its story.
Before: native snap; slanted drags scroll the calendar. This PR pages one month; 35° stays, 60° moves the sheet; it rubber-bands, mirrors in RTL, moves while blocked, and dips under Reduce Motion (the base through #8496).
Problem and solution fit
Under a finger,
MonthPager(#8146'susePager, #8178's values):setContentOffset's 0.3 s sinusoid; a second tap retargets.design:reduced-motion).Current specification
Settled (cixzhang, Oct 25):
spec:AST-094FR36, FR2, FR17.component:DateInput(draft): AR8's PageUp/PageDown gap closes.Public delta and caller burden
analyze-pr.js), import gzip, base → head: DateInput 132.5 → 148.8 KB, DateTimeInput 143.8 → 161.6 KB, PowerSearch 199.0 → 215.6 KB; component CSS +10 to +11 B.Design and alternatives
Rejected:
scroll-snap-stop: always(keeps WebKit's deceleration).Writing rubric results
R1 narration: AR8's gap sentence removed; PR2 and AR8's exception present tense. R2–R6: none.
Scope
Testing
motion/MEASUREMENTS.md): the first move past 10 px either way decides, the UIKit samples (10, 11) and (10.7, 12.7) going to the grid and (9.7, 11.3) and (8, 10.3) to the sheet; a fast 55° drag whose first move lands 14 px sideways is the grid's and a slow one the sheet's; moves before the decision reach neither, so the sheet's 8 px promotion waits. jsdom, and the Chromium and WebKit spec, whose fast steep drag failed before (the sheet dropped 102 px).spec:AST-094FR2): a trackpad swipe's step and quiet time are labelled guesses (M-DP02; the rig cannot drive a trackpad). AR8 names Shift+PageUp/PageDown as a named exception (spec:AST-020FR15);pnpm check:knowledgepasses.MonthPager.interaction.browser.spec.ts, Chromium 17/17 and WebKit (the trackpad case skips: Playwright sends no wheel events to mobile WebKit):DateInputTouch.test.tsx: the month/year toggle leaves the days alone (fails on the base: threeuseCalendarDayspasses); the native scroller's Reduce Motion step (fix(DateInput, DateTimeInput): the month scroller's steps follow design:reduced-motion #8496's) is instant on a fine pointer and dips on a coarse one; a touch-screen laptop with a pinned bottom sheet keeps the native scroller; PageUp/PageDown/Shift.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): … → #8159 → #8178 → #8179 → #8146 → #8496 → this PR (the full order is in #8156).