Repository navigation
fix(DateInput, DateTimeInput): the month scroller's steps follow design:reduced-motion - #8496
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
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateInput · Storybook | 100.5 kB (+1.3 kB) · 15.3 kB CSS (0) | 3,687 (+38) | ✓ · ✓ · ✓ |
| lab/MobilePowerSearch | 166.2 kB (+1.2 kB) · 1.2 kB CSS (0) | — | — · — · — |
| DateTimeInput | 112.4 kB (+1.2 kB) · 15.8 kB CSS (0) | — | — · — · — |
1 more component
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch | 181.4 kB (+402 B) · 20.5 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 5.82 MB packed (+1.7 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 18 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.
Requesting changes for scope: the month-scroller fix itself follows design:reduced-motion (with Reduce Motion on, the month changes at once with a mouse and through a 200 ms opacity dip on touch, and its new tests pass), but this pull request currently carries 22,543 changed lines because it includes #8076 and its predecessors. Please land those through their own pull requests first, then update this branch from main so it contains only the fix's four files. That update also brings in the DropdownMenu SSR test fix behind the failing test-ui (2/4) shard.
[Automated review]
meta#8496 carries the month scroller's Reduce Motion steps, so this pull request keeps only PageUp/PageDown and the title following them in the scroller; its mouse Reduce Motion test is meta#8496's now, and under a finger the calendar is the pager, whose own spec proves its dip. The gesture line no longer claims a pan early, so the pager's pan claims its touch when it begins.
There was a problem hiding this comment.
Requesting changes: the month-scroller fix itself follows design:reduced-motion (with Reduce Motion on, the month changes at once with a mouse and through a 200 ms opacity dip on touch), but this pull request still carries the #8076 and #8534 stacks, about 19,000 changed lines beyond the fix. Please land those through their own pull requests, then update this branch from main so it contains only MonthScroller.tsx, its two tests and its changeset. Also call stopDip() at the top of scrollToMonth: today a Reset to a distant month tapped within 100 ms of an arrow step is undone when the earlier dip lands (MonthScroller.tsx:416-468).
[Automated review]
| // A second step within the dip replaces the first: only its scroll | ||
| // lands, at its own dip's middle. | ||
| stopDip(); |
There was a problem hiding this comment.
Only a newer dip replaces a running one here. The far branch above (Math.abs(row - centerRowRef.current) > OVERSCAN) and the 'auto' branch below never call stopDip(), so a Reset to a distant month tapped within 100 ms of an arrow step is undone when this timer's land() runs. Calling stopDip() at the top of scrollToMonth lets every newer step win.
There was a problem hiding this comment.
The dip's landing ran on its own timer, and only a newer dip stopped it, so a jump to a distant month (Reset) or a step at once made in the dip's first half was undone when the dip landed. Fixed in b8a7bd9: scrollToMonth calls stopDip() first, so any newer step replaces a running dip. Two new cases in MonthScroller.test.tsx make a jump to a distant month and a step at once 40 ms into a dip; both failed before (the dip landed after them and scrolled back to its own month) and pass now.
There was a problem hiding this comment.
Requesting changes for scope: the month-scroller fix itself follows design:reduced-motion (under Reduce Motion the month now changes at once with a mouse and through a 200 ms opacity dip on touch, and a newer step cancels a running dip), but this pull request still carries the #8076 and #8534 stacks, about 19,500 changed lines beyond the fix. Please land those through their own pull requests, then update this branch from main so it contains only MonthScroller.tsx, its two tests and its changeset. Before the stacks land, two of their changes need to match current records: Lab's paths to core's touch modules should go through @astryxdesign/core/internal/touch (architecture:react-component-runtime INV13), and the flex-row BottomSheet dialog host needs component:BottomSheet's "holds the panel at the block end with its own grid" amended, or the grid kept.
[Automated review]
|
Both points are in the stack, not this pull request's own change (https://github.com/vjeux/astryx/compare/77ec95210f..b8a7bd94fe): #8076 moves Lab onto |
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the month-scroller fix does what Reduce Motion asks, but this branch also carries about 16,000 lines of the still-open #8076 and #8534, so please land those on their own and update this branch from main until it holds only the four fix files. One fix is still needed in MonthScroller.tsx: the Reduce Motion land() does not clear steeredRowRef, so after the month wheels close and a step lands, a trackpad scroll stops updating the header month; clearing it at the top of land(), with a test for that sequence, resolves it.
[Automated review]
|
The Reduce Motion |
There was a problem hiding this comment.
Changes requested for scope: the month-scroller fix does what Reduce Motion asks (the month changes at once with a mouse and through a 200 ms opacity dip on touch, and the landing now clears the earlier steer, with a test), but this branch still carries about 14,200 lines of the open #8076 and #8534. Please land those through their own pull requests, then update this branch from main so it holds only MonthScroller.tsx, its two tests and its changeset.
[Automated review]
cdbe481 to
9b8d6c0
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
|
/approve-spec 9b8d6c0 |
There was a problem hiding this comment.
Changes requested, for one release-note sentence. The month-scroller fix itself follows design:reduced-motion: with Reduce Motion on, a month step now changes at once with a mouse, trackpad or desktop keyboard, and through one 200 ms opacity dip on a touch screen, and the new tests fail before the change and pass after it.
.changeset/month-scroller-reduced-motion.md: "closing the sheet during the dip scrolls nothing" does not hold, because closing the sheet leaves the calendar mounted and the dip still lands on the stepped month as the sheet leaves; end the sentence at "replaces the first", or say "unmounting the calendar during the dip scrolls nothing".
Two optional comment suggestions are inline.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] With Reduce Motion on (`design:reduced-motion`), the month arrows in the DateInput and DateTimeInput bottom-sheet calendars, Reset, and a keyboard move into another month no longer scroll the months: with a mouse or a keyboard the month changes at once, and on a touch screen it changes through a short opacity dip. A second step during the dip replaces the first, and closing the sheet during the dip scrolls nothing. |
There was a problem hiding this comment.
"closing the sheet during the dip scrolls nothing" does not hold: the bottom sheet keeps its content mounted when it closes, so MonthScroller stays mounted and the dip's timer still lands. With Reduce Motion on touch, tapping Next and then Save 36 ms later scrolls the calendar to the stepped month about 95 ms after Save, while the sheet is leaving. That landing is the right behavior (the calendar ends on the month the header shows); only the sentence is wrong. Suggested ending: "A second step during the dip replaces the first." or "…, and unmounting the calendar during the dip scrolls nothing."
There was a problem hiding this comment.
At 6728ff1: Fixed. The sentence now ends "and unmounting the calendar during the dip scrolls nothing", which MonthScroller.test.tsx "scrolls nothing once unmounted within the dip" covers; the landing after Save is kept as the right behavior.
| // Under reduced motion the month changes without travel. It is immediate | ||
| // with a mouse or a keyboard. On a touch screen an opacity dip plays, and | ||
| // the scroll lands under it (reducedMotionFade). |
There was a problem hiding this comment.
Non-blocking: the branch is decided by the primary pointer, not the input, so a keyboard step on a tablet dips. A shorter, exact version:
// Under reduced motion the month changes without travel: at once on a fine
// primary pointer, under an opacity dip on a coarse one (reducedMotionFade).There was a problem hiding this comment.
At 6728ff1: Done, with your wording: the branch follows the primary pointer, fine or coarse. The changeset now says "on a computer with a mouse or trackpad" against "on a touch screen" for the same reason.
| // re-asserts the month already in view. The landing's own scroll | ||
| // reports nothing, as it lands on the centre row. |
There was a problem hiding this comment.
Non-blocking: this repeats the comment above ("A scroll that lands on the centre row reports nothing"); one of the two is enough.
There was a problem hiding this comment.
At 6728ff1: Done: the repeated "the landing's own scroll reports nothing" is gone; the comment above keeps it.
…gn:reduced-motion Under reduced motion a step the bottom-sheet calendar's month scroller is asked for, by the month arrows, Reset or a keyboard move into another month, changes the month without travel, through core's reducedMotionFade: at once with a mouse or a keyboard (design:reduced-motion DR3), and through a 200 ms opacity dip on a touch screen, the scroll landing at the dip's middle (DR2). The jump lands once the window of mounted months has moved to the target, as a far jump already does, so nothing moves the window afterwards: WebKit re-snapped a jump that the window then followed, one month further on every frame. A second step within the dip replaces the first, and the dip's timer and animation end with the scroller. Without reduced motion nothing changes.
…duce Motion dip A dip's landing ran on its timer whatever came after it. A jump to a distant month, or a step at once, made within the dip's first half was undone when the dip landed: a Reset tapped right after an arrow step ended on the arrow's month. scrollToMonth now stops a running dip first, so the newer step wins whichever way it moves.
…clears an earlier steer The month scroller ignores the scrolls of a steer until it arrives. The re-assert when the month wheels close steers onto the month already in view, so its scroll never arrives; a step that then lands under Reduce Motion left that steer in place, and every later scroll, a trackpad's included, stopped updating the header month. A landing is the newest steer, so it clears the earlier one; its own scroll lands on the centre row and reports nothing.
…, and the travel by positions On a loaded machine the page's frames come too far apart to show a 200 ms dip's darkest frame: in Chromium at four workers on two cores the darkest sampled opacity was 0.54 to 0.89 in 5 of 10 runs. The touch case now reads the dip from its keyframes and timing as it is created, and waits for it to end. The mouse case counted frames of the smooth scroll: in WebKit's CI container each of its frames takes 67 to 83 ms, so its 220 to 258 ms show two or three offsets in between, and one run in ten showed two. It now asserts travel as at least one offset between the two months. Red-first: with every frame costing 110 ms (mouse) and a 180 ms block from the dip's creation (touch), the old proofs fail 5/5 in WebKit's CI lane (the touch case 5/5 in Chromium too); the new ones pass 5/5 in both.
9b8d6c0 to
6728ff1
Compare
|
/approve-spec 6728ff1 |
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Recommending approval. With Reduce Motion on, a month step in the DateInput and DateTimeInput sheet calendars now changes at once on a computer and through one 200 ms opacity dip on a touch screen, as design:reduced-motion asks. The new unit tests fail before the change and pass after it, and the changeset now describes the unmount case correctly. One optional comment cut is inline.
[Automated review]
| // A second step within the dip has replaced the first (above): only | ||
| // its scroll lands, at its own dip's middle. |
There was a problem hiding this comment.
Non-blocking: this repeats the comment above stopDip() at the top of scrollToMonth and the fade.duration / 2 below it. Deleting these two lines loses nothing; the comment above stopDip() already gives the reason.
Batch: B4 · Wheel / picker (DateInput, TimeInput, DateTimeInput)
Sits on main, independent of the wheel stack:
MonthScroller.tsx, its two test files and its changeset.User impact
With Reduce Motion on, the month arrows, Reset and keyboard moves in the DateInput and DateTimeInput sheet calendars still slide the months sideways, with a mouse and on a touch screen.
Problem and solution fit
scrollTo({behavior: 'smooth'})whatever the preference.design:reduced-motionasks for a change without travel: immediate with a mouse or a keyboard (DR3), and a short cross-fade on a touch screen (DR2).Now the month changes without travel. It is immediate on a fine primary pointer (a computer with a mouse or trackpad), and on a coarse one (a touch screen) it goes through an opacity dip of 200 ms or less.
Expected behavior and authority
design:reduced-motionDR2 and DR3, through core'sreducedMotionFade.Smallest restoration
MonthScroller.tsx: a step asksreducedMotionFade, then jumps (mouse, keyboard) or dips (touch screen).Evidence
MonthScroller.test.tsxcases (a second step, a jump or an immediate step within the dip; a trackpad scroll after an earlier steer that never arrived; an unmount within the dip) and the browser spec's two Reduce Motion cases fail on main.Scope
Earlier review threads: r4385320519 is answered on its thread. This pull request now holds only the four files the Oct 29 review asked for.
Testing
Local gates at 6728ff1 (its code is the same as at 9b8d6c0, where the browser specs ran, and at d1720a1f1e, where the unit suite ran; since then only comments and the changeset's last sentence changed):
vitest runover DateInput and DateTimeInput: 12 files, 608 tests passed.pnpm -F @astryxdesign/core typecheck,check:repo, strict ESLint and Prettier on the touched files.--checkreports only drift that main has at e0548a6 (ChatComposer, Item, Token, layerSurfacePress), in files this pull request does not touch.review-global-baselines.mjs, triggers accessibility, behavior, layout, public-api, theming): 221 claims match. This change touches none of their surfaces: no styles, tokens, new strings, sizes, layer runtime or public API.MonthScroller.interaction.browser.spec.tsin Chromium and WebKit: at 9b8d6c0: Chromium ×3, 9/9; Chromium pinned to two exclusive cores ×3, 9/9; WebKit in the CI lane's profile (podman--cpus=4,CI=1,LP_NUM_THREADS=4, 1 worker) ×10, 30/30.analyze-pr.js), import gzip, main → head: DateInput 99.2 → 100.5 KB (+1.3), DateTimeInput 111.2 → 112.4 KB (+1.2), PowerSearch 181.0 → 181.4 KB (+0.4), MobilePowerSearch (Lab) 165.0 → 166.2 KB (+1.2). Every CSS size is unchanged. The growth is core'sreducedMotionFade, which the month scroller now reaches.