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
meta-cla
Bot
added
the
CLA Signed
d02a6db to
27cd25c
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs a spec decision before it can be approved: no current spec on main covers the touch picker wheel looping any list of two or more rows (copies around the whole cylinder, a tap turning onto a copy, arrow keys stepping across the seam), so an engineering owner needs to identify or provide the governing record and record the decision by commenting /approve-spec 27cd25cf0b1e72adf13d6da7e247ba213d6fe368 on this pull request; the author, as an engineering owner, may do so. Separately from that decision, three changes are needed:
- Land #8361 and its predecessors first, or retarget, so this pull request's diff is its own 362-line change rather than about 40,700 authored lines.
.changeset/wheel-short-loop.mdannounces a[feat]for 10- to 30-minute minute columns, but no shipped minute column has those steps or passesisLooping, so tag it[chore] Internal:and carry the user-facing note with the consumer.- In
PickerWheel'smoveBy, turning by the wholefound − frommakes PageDown on a 2-row loop, or an arrow key when only the current row is enabled, spin whole turns back onto the same value; staying put when no other value is found and folding the travel into one turn would fix it.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Under a finger, a looping picker wheel with fewer rows than its cylinder shows now repeats around the whole cylinder, as UIPickerView does. This applies to a minute column every 10, 15, 20 or 30 minutes. Before, a list of fewer than 11 rows ignored `isLooping` and stopped at its ends. Now a drag or a fling runs on through the seam, a key steps in its own direction across it, and a tap on a copy turns the wheel onto that copy, committing nothing when it holds the same value. Each value is still listed once for assistive technology. |
There was a problem hiding this comment.
This [feat] note tells consumers that a 10-, 15-, 20- or 30-minute minute column now loops, but at this head every TimeInput and DateTimeInput minute wheel is a fixed 60-row list, none passes isLooping, and Wheel isn't exported. Consider [chore] Internal:, like wheel-listbox-hook.md, and carrying the user-facing note with the change that passes isLooping.
| } | ||
| // The step's own direction, even on a loop shorter than the cylinder, | ||
| // where the same row also sits the other way round. | ||
| turnToPosition(headingPosition + (found - from), offset); |
There was a problem hiding this comment.
Turning by the whole found - from spins whole turns when found lands on a copy of the starting row: PageDown (PAGE_ROWS 10) on a 2-row loop turns 10 rows back onto the same value, and ArrowDown with only the current row enabled turns a full revolution (60 rows on a looping minute column), where the removed turnTo(wrapIndex(found, count), ...) stayed put. Returning when wrapIndex(found, count) === from, and otherwise folding the travel to at most one turn in the key's direction, keeps this commit's direction fix for the 2-row case.
There was a problem hiding this comment.
Fixed in a294aad: a target that folds back onto the row the wheel is heading to stays put, and any other travels less than one turn, still in the key's direction, so the two-row loop keeps stepping the way the key points. PickerWheel.test.tsx: PageDown on a two-row loop and an arrow key with no other row enabled; both turned onto a copy before.
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 | +28.8KB |
+14B |
| core/DateTimeInput | +28.1KB |
+15B |
| core/PowerSearch | +28.5KB |
+14B |
| core/TimeInput | +27.6KB |
+30B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | +21B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
27cd25c to
bccf372
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: no current spec on main covers the touch picker wheel looping any list of two or more rows (copies around the whole cylinder, a tap turning onto a copy, keys stepping across the seam), so an engineering owner needs to identify or provide the governing record and record that decision by commenting /approve-spec bccf37254384ce3f5fd8248af1349548f25de01c on this pull request; the author, as an engineering owner, may do so. Independently of that decision, three changes are needed:
- Land #8361 and its predecessors first, or retarget, so this pull request's diff is its own 362-line change rather than about 43,200 authored lines.
.changeset/wheel-short-loop.mdannounces a[feat]for 10- to 30-minute minute columns, but no shipped minute column has those steps or passesisLooping, so tag it[chore] Internal:and carry the user-facing note with the consumer.- In
PickerWheel'smoveBy, turning by the wholefound − frommakes PageDown on a 2-row loop, or an arrow key when only the current row is enabled, spin whole turns back onto the same value; staying put when no other value is found and folding the travel into at most one turn would fix it.
[Automated review]
bccf372 to
9a781ce
Compare
vjeux
changed the title
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
…-nearest-valid
…-nearest-valid
…el-nearest-valid
…e turn - The wheel reads the pointer type on the way down. usePointerDragScroll stops a primary mouse press from bubbling, so after a touch a mouse drag's travel still cross-faded on a fine pointer under reduced motion. - With meta#8483 merged in, the travel waits for a mouse drag's release instead of running whenever the hand pauses; a test covers it here. - DateInput's Owns line claims the shared wheel's mechanics; the TimeInput and DateTimeInput records own their sheets' time wheels. - FR13 and DateTimeInput FR8 say a shown wheel turns onto a value changed from outside, which the code and its tests now cite.
…he turn) into uikit/c21-wheel
…ps its mark after a nudge - A value changed from outside while the wheel is shown (a bound clamping the time after another wheel turned) turns the wheel onto its row, as UIDatePicker does and FR13 now says. The first park when the wheel is shown stays at once. - A motion that ends on the row it began on (a nudge that eases back, a drag off a row and back) gives the band's mark back to that row: the player forgets its last report when a motion starts, so the row it ends on is reported again. - The ctrl-wheel note cites FR43 (a ctrl-wheel is a pinch) and FR10 (with no pinch of its own, the wheel leaves it to the page's zoom).
…sheet on the desktop viewport This file's mouse cases run on a desktop viewport, where the field shows its popover unless it asks for the sheet; the case meta#8483 added now asks for it, as its neighbours do.
…e new foundation) into uikit/c21-wheel-proofs
… FR5 names The coast's proof now blocks the main thread for 1 s, the length spec:AST-094 FR5's verification names, ending while the coast is still over a row from rest so every frame moves a pixel or more. It follows the row that crosses the band halfway through, which stays in sight the whole second. The WebKit skip says what covers Mobile Safari instead: an iOS simulator capture through a 530 ms block.
…nto uikit/c21-wheel-listbox
The four Effects without a note now say what they synchronize: the latest props for callbacks attached once, the band's mark with the row the motion reports, the motion's end with the wheel's, and the latest touch-down handler.
…kit/c21-wheel-short-loop
…nd its seam - A rest on a disabled row measures the distance to the nearest enabled row around the loop, with a tie going toward the committed row, and turns the shorter signed way. It no longer walks the list as a straight line, which sent a rest beside the seam the long way round (a 4-row loop with :00 and :15 disabled, rested on :00 from :45, turned on to :30). - A key's search for the next enabled row stops half a turn from where it started, so it never turns the long way round onto the far side of a disabled stretch (up from January, with only January and February enabled, stays on January). - A short loop repaints the band's row when the options change, so a value whose enabled state changes under a resting wheel takes its new colour.
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
…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.
There was a problem hiding this comment.
Approval waits on a spec decision: no current spec on main covers the touch picker wheel's isLooping (any list of two or more rows looping through its seam, a short list repeated down the column as hidden copies, taps onto a copy, keys across the seam, and a rest beside the seam turning the shorter way), and the diff also carries unapproved draft requirements in the DateInput and DateTimeInput records (DateInput FR13, its DEC-1 and its ownership of the sheet wheel, and DateTimeInput FR8). An engineering owner can identify or provide the governing spec and approve or reject those requirements by commenting /approve-spec 5292d5e15d93a23da2857ee98330429f23212155 on this pull request; the author may do so as an engineering owner. Separately, the diff against main carries the whole open stack under #8361 (about 35,900 authored lines), so please land that stack first or retarget this pull request so it holds only its own commits. The band-mark issues raised on earlier revisions are fixed here.
[Automated review]
…nto uikit/c21-wheel-rest-and-spin
…nto uikit/c21-wheel-rest-and-spin
…heirs) into uikit/c21-wheel-rest-and-spin
|
Mapped to 5292d5e:
Open: no current spec covers |
Depends on #8361. Review only this PR's change: vjeux/astryx@eeeea52...f3ce064
User need
On iOS, a time picker's minute and hour columns loop: past 59 comes 00. With a 15-minute interval, the minute column has only 4 rows, and iOS repeats them down the whole column. Astryx's touch wheel didn't loop, so a short list stopped at its ends and left half the column empty.
Problem and solution fit
What led to it: Astryx's UIKit-parity work (
spec:AST-094). Recording UIDatePicker in the UIKit lab (iOS 26.2) at 15- and 30-minute intervals showed the copies filling the cylinder, a flick running through the seam, and a tap onto a copy two rows away. TimeInput's minute interval (#8179) needs it.isLooping(internal): the last row is followed by the first, for any list of two or more rows.Current specification
spec:AST-094FR1, from the UIKit lab recordings above.Public delta and caller burden
No public API:
isLoopingis internal, passed by TimeInput's minute column at 10- to 30-minute intervals (#8179). A mouse is unchanged.Design and alternatives
Evidence
Unit tests replay the recorded cases: the column at :45 and :30, a tap two rows down, ArrowDown at :45 landing on :00, and a flick through the seam. Two fail without their fix: a key that would turn a whole revolution stays put, and only the band's copy carries the selected mark, a disabled copy staying grey.
Scope
Testing
pnpm exec vitest run packages/core/src/DateInput/PickerWheel.test.tsx: the looping cases pass, each matching what iOS did in the recordings.