Repository navigation
chore(core): Core's internal touch entry re-exports the action-group tracking hook, extended for Lab's TabBar, and a tab-bar string joins the catalog - #9699
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| lab/CommentThread | 296.2 kB (+94 B) · 2.4 kB CSS (0) | — |
| lab/Comments | 301.8 kB (+72 B) · 2.5 kB CSS (0) | — |
| lab/Schedule | 99.3 kB (+64 B) · 3.9 kB CSS (0) | — |
5 more components
| Component | Size | LoC |
|---|---|---|
| ButtonGroup | 40.1 kB (+60 B) · 3.7 kB CSS (0) | — |
| AlertDialog | 95.8 kB (+59 B) · 13.7 kB CSS (0) | — |
| lab/PageControl | 71.7 kB (+15 B) · 709 B CSS (0) | — |
| lab/NavigationScreen | 78.4 kB (+10 B) · 1.5 kB CSS (0) | — |
| lab/NavigationStack | 78.4 kB (+10 B) · 1.5 kB CSS (0) | — |
Package size: @astryxdesign/core 6.75 MB packed (+897 B); 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.
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Current run: CI concluded cancelled; parts of this report may be missing.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
Requesting changes. The Core part itself, seven named re-exports on the internal touch entry and three catalog strings, follows the current internal-entry and catalog rules, and its tests pass. What blocks is what the diff against main carries.
- The diff includes #9399's two commits, which make it 7,261 changed lines (51 of them this pull request's own); land #9399 first and rebase, so this pull request contains only its own commit.
- SearchField's
armSwallow(packages/lab/src/SearchField/SearchFieldTouch.ts) adds its own document listeners to swallow a claimed tap's late mouse events, a second stray-click swallower beside Core's click guard, which INV13 indocs/architecture/react-component-runtime.mdallows only once; extendutils/clickGuard.tsto cover the compatibilitymousedownandmouseupand call it instead. .changeset/core-internal-entries-lab-tabbar.mdsays Lab's TabBar reaches these modules and that TabBar's strings join the catalog, but no TabBar exists at this head; describe the re-exports and the strings themselves.
Two non-blocking comment suggestions are inline.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] Lab's experimental TabBar reaches Core's pan recognizer, press tracker, finger-scroll reader, selection lock, touch query, rubber band and UIKit gesture values through Core's internal `@astryxdesign/core/internal/touch` entry. That entry stays outside the public API and its compatibility promise. TabBar's strings join Core's message catalog. |
There was a problem hiding this comment.
No TabBar exists at this head (it arrives in #9700 and #9701), so this release note announces a consumer the release may not contain. Suggested wording:
[experimental] Core's internal `@astryxdesign/core/internal/touch` entry re-exports the pan recognizer, press tracker, finger-scroll reader, selection lock, touch query, rubber band and UIKit gesture values for Lab components. Three tab-bar strings join Core's message catalog.
There was a problem hiding this comment.
Fixed in b5b0aaa with your wording: the note describes the entry's re-exports and the three strings, and names no consumer.
| const armSwallow = useCallback((doc: Document) => { | ||
| disarmRef.current?.(); | ||
| let sawGuard = false; | ||
| const swallow = (event: Event) => { | ||
| if (event.type === 'click' && (event as MouseEvent).detail === 0) { | ||
| return; | ||
| } | ||
| if (isClickGuardArmed(doc)) { | ||
| sawGuard = true; | ||
| } else if (sawGuard) { | ||
| disarm(); | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (event.type === 'click') { | ||
| disarm(); | ||
| } | ||
| }; | ||
| const types = ['mousedown', 'mouseup', 'click'] as const; | ||
| const ends = ['touchstart', 'pointerdown', 'keydown'] as const; | ||
| const disarm = () => { | ||
| for (const type of types) { | ||
| doc.removeEventListener(type, swallow, true); | ||
| } | ||
| for (const type of ends) { | ||
| doc.removeEventListener(type, disarm, true); | ||
| } | ||
| if (disarmRef.current === disarm) { | ||
| disarmRef.current = null; | ||
| } | ||
| }; | ||
| for (const type of types) { | ||
| doc.addEventListener(type, swallow, true); | ||
| } | ||
| for (const type of ends) { | ||
| doc.addEventListener(type, disarm, true); | ||
| } | ||
| disarmRef.current = disarm; | ||
| }, []); | ||
| // Unmount releases a pending swallow's document listeners. | ||
| useEffect(() => () => disarmRef.current?.(), []); |
There was a problem hiding this comment.
These document listeners are a second stray-click swallower beside Core's click guard (packages/core/src/utils/clickGuard.ts), and INV13 in docs/architecture/react-component-runtime.md allows only one. Extending the click guard to cover a claimed tap's compatibility mousedown and mouseup, and arming it from this tap through the internal entry, would let armSwallow, disarmRef and the unmount Effect go.
| * @input The gesture core: the arena, the recognizers and their geometry, | ||
| * React hook, press tracker, finger-scroll reader and text-selection lock. | ||
| * The animator, the cubic-bézier solver, the rubber band and UIKit's gesture | ||
| * and timing tables. The touch-presentation hook and its query, the | ||
| * writing-direction read, and whether the click guard is armed. |
There was a problem hiding this comment.
Non-blocking: INV29 keeps a file header to about 12 lines, and this one is now 23. A shorter @input:
* @input Core's gesture, motion and touch modules that Lab components useThere was a problem hiding this comment.
Taken in b5b0aaa: @input is now your one line, and the header is down from 23 to 19 lines. The rest is the @position block's record citations, which stay until a record links the file.
| // A tap on the text of a field at the bottom of the screen (`bottom`, | ||
| // `tab`) focuses it without the browser's reveal (`preventScroll`). The | ||
| // browser would otherwise pan the whole page up to show a field the | ||
| // keyboard is about to cover. The tap is the gesture system's: recognizing | ||
| // it cancels the browser's own focus for that touch. Taps on the buttons, | ||
| // and taps once the text has focus, stay the browser's. | ||
| // The claim applies only where the keyboard covers the page, Chrome on | ||
| // Android included. There the browser's own tap scrolls the document once | ||
| // its keyboard is up. The claimed tap's focus brings the keyboard up and | ||
| // pans nothing (measured on the Android emulator). | ||
| // The recognizer attaches only where it claims: any other field keeps the | ||
| // browser's own touch-action and tap. | ||
| // It takes a finger and a pen, because Safari reveals a field for both. A | ||
| // mouse press stays the browser's, which places the caret and selects text | ||
| // where it lands. The claim is touch work, so it attaches only where the | ||
| // primary pointer is coarse. Under a fine one, as in desktop Safari or | ||
| // Firefox, no software keyboard rises. There a mouse or a pen tablet keeps | ||
| // the browser's own focus and click. | ||
| // The keyboard check reads the window that the touch presentation reads. | ||
| // `isTouch` is false on the server and while hydrating, so no markup | ||
| // differs. |
There was a problem hiding this comment.
Non-blocking: this comment covers seven topics in 21 lines (INV29: one topic per sentence). A shorter version that keeps the reasons:
// A tap on the text focuses the field with preventScroll. The browser's own
// focus would pan the page under the keyboard. The claim attaches only under
// a coarse pointer where the keyboard covers the page. It takes touch and pen.
// A mouse keeps the browser's focus and caret.There was a problem hiding this comment.
This comment is #9399's (SearchField), carried here only by the stack. I've passed your shorter version to that PR's thread rather than change its code here.
b86bd24 to
b5b0aaa
Compare
|
On the stack: this PR's own commit is the 51-line change at vjeux/astryx@1748182...b5b0aaa. The rest of the diff is #9399, which this branch sits on because both change |
There was a problem hiding this comment.
Requesting changes. The Core part itself, seven named re-exports on the internal touch entry and three catalog strings, follows the internal-entry and catalog rules, its tests pass, and the TabBar changeset now describes exactly what the head contains. What blocks is what the diff against main carries from #9399.
- The diff includes #9399's two commits, which make it 7,257 changed lines (47 of them this pull request's own); land #9399 first and rebase, so this pull request contains only its own commit.
- SearchField's
armSwallow(packages/lab/src/SearchField/SearchFieldTouch.ts) adds its own document listeners to swallow a claimed tap's late mouse events, a second stray-click swallower beside Core's click guard, which INV13 indocs/architecture/react-component-runtime.mdallows only once; extendutils/clickGuard.tsto cover the compatibilitymousedownandmouseupand call it instead. - SearchField's resize handling in the same file creates its own
ResizeObserverper field instead of callingobserveResizefrom@astryxdesign/core/utils; subscribe through the shared observer, and extend it if the field's own window is the reason.
Both SearchField items live in #9399's code and leave this diff with the rebase.
[Automated review]
| useLayoutEffect(() => { | ||
| const field = fieldRef.current; | ||
| const Observer = field?.ownerDocument.defaultView?.ResizeObserver; | ||
| // The tab bar lays out and moves a tab field's box; it has no parts. | ||
| if (field == null || Observer == null || placement === 'tab') { | ||
| return; | ||
| } | ||
| const observer = new Observer(() => { | ||
| if (isAnimating.current || settledRef.current === null) { | ||
| return; | ||
| } | ||
| if (isBottom) { | ||
| placeBottom(bottomPoseFor(settledRef.current, bottomShape().width)); | ||
| } else { | ||
| place(poseFor(settledRef.current, geometry())); | ||
| } | ||
| }); | ||
| observer.observe(field); | ||
| return () => observer.disconnect(); |
There was a problem hiding this comment.
This creates one ResizeObserver per field. observeResize in @astryxdesign/core/utils (packages/core/src/utils/sharedResizeObserver.ts) already owns resize observation, and SearchField.tsx already imports from that entry. Subscribing with observeResize(field, …) and returning its unsubscribe would drop the per-instance observer; if the field's own window is the reason, extending observeResize for it keeps one path.
There was a problem hiding this comment.
This is #9399's code too (SearchField), carried here only by the stack, and I've passed it to that PR's author. This PR's own commit has no observer.
b5b0aaa to
62b989d
Compare
There was a problem hiding this comment.
Requesting changes. The Core part of this pull request (eight named re-exports on the internal touch entry, the widened useFingerScroll type and three catalog strings) follows the internal-entry and catalog rules, and its tests pass. What blocks is what the diff against main still carries from #9399.
- The diff includes #9399's two commits, which make it 7,416 changed lines (76 of them this pull request's own); land #9399 first and rebase, so this pull request contains only its own commit.
- Those commits bring Lab's SearchField with its strings,
astryx discoverentry, stories and tests, which this pull request's title and body do not cover and which belong in #9399, so they need to leave this diff.
The two earlier SearchField findings (the second stray-click swallower and the per-field ResizeObserver) are fixed at this head. Two non-blocking notes are inline.
[Automated review]
| * the document (or a ref that holds it) for the page's own scroll. Returns a | ||
| * reader, stable across renders: `true` while the scroller's scroll is a | ||
| * finger's. | ||
| * Internal: not exported from the package. |
There was a problem hiding this comment.
Non-blocking: this commit re-exports the hook from @astryxdesign/core/internal/touch, so "not exported from the package" no longer holds. Suggested:
* Internal: outside Core, only `@astryxdesign/core/internal/touch` reaches it.There was a problem hiding this comment.
Good catch. I'll take your wording in the push that rebases this onto main once #9399 lands, so it rides with the change this round can't make yet.
| return {duration, at, velocity: t => slope(at, t)}; | ||
| } | ||
|
|
||
| /** A curve times a constant (a direction). */ |
There was a problem hiding this comment.
Non-blocking (INV29, #9399's code): this JSDoc restates scaled's one-line body. Dropping it loses nothing.
There was a problem hiding this comment.
This is #9399's code, carried here only by the stack. I've passed it to that PR's author.
62b989d to
53c7f49
Compare
There was a problem hiding this comment.
Requesting changes. This pull request's own commit (eight named re-exports on the internal touch entry, the widened useFingerScroll type and three catalog strings) follows the internal-entry and catalog rules, and its new test and type cases fail before the change and pass after it. What blocks is what the diff against main still carries from #9399.
- The diff includes #9399's two commits, 7,371 of its 7,449 changed lines, and this copy is already behind #9399's own head; land #9399 first and rebase onto
main, so this pull request holds only its own 78-line commit. - Lab's SearchField, its stories, strings, discover entry and registry route belong to #9399 and have no place in a pull request about the touch entry's re-exports; they leave with that rebase.
Non-blocking, for #9399: the local translate(...) helper in SearchFieldBottom.interaction.browser.spec.ts makes the i18n catalog check list nine unverifiable key references; renaming it removes the noise.
[Automated review]
53c7f49 to
918491f
Compare
|
Restacked on #9754 instead of #9399, since #9399 won't merge. This PR's own commit is now only what TabBar adds to Core's internal touch entry: the press tracker, the action-group tracker, the finger-scroll reader and the selection lock, plus the widened |
There was a problem hiding this comment.
Requesting changes for one small fix. The new internal re-exports, the widened useFingerScroll type and the three catalog strings follow the internal-entry and catalog rules.
.changeset/core-internal-entries-lab-tabbar.mdis tagged[experimental], which lists this internal-entry change under "Experimental APIs" in Core's release notes, while AST-017 FR21 and FR22 keep./internal/*outside experimental status; please retag it[chore], as the stacked changeset does.
Non-blocking: the @output header in packages/core/src/internal/touch.ts and the @input header in packages/core/src/gestures/useFingerScroll.ts no longer describe everything they now cover.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] Core's internal `@astryxdesign/core/internal/touch` entry re-exports the press tracker, action-group tracker, finger-scroll reader and selection lock for Lab components. The finger-scroll reader also takes a ref that holds a document. Three tab-bar strings join Core's message catalog. |
There was a problem hiding this comment.
[experimental] files this under "Experimental APIs" in Core's release notes, but an ./internal/* entry is outside experimental status (AST-017 FR21 and FR22). Please retag it [chore], as core-internal-touch-entry-lab-animator.md does; the patch bump stays.
There was a problem hiding this comment.
Retagged [chore], as #9754's changeset is; the patch bump stays.
918491f to
a88504b
Compare
|
Changes in this round:
|
|
/approve-spec a88504b |
|
/approve-spec 4bbb9ca |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is a draft, and it runs once the pull request is marked ready for review.
The hook's new paintsHighlight option and press callbacks, their re-export from Core's internal touch entry and the badged-tab string follow the action-group, internal-entry and catalog rules, and AlertDialog, the hook's only Core caller, is unchanged. #9700 still sits on this branch's previous head, so it needs a restack before the two line up.
- Miss: (non-blocking)
onPressCancel's doc comment can also name the group unmounting mid-press, which the cleanup reports as a cancel (see the inline note). - Miss: (non-blocking)
LAB_ONLY_NAMESinscripts/touch-support-bundle.test.mjscan listuseActionGroupTracking, as the entry's sync note asks (see the inline note).
[Automated review]
| * The press ended with nothing acting: a cancel, a second finger, or | ||
| * tracking turned off. |
There was a problem hiding this comment.
Non-blocking: the Effect cleanup (lines 132-139) also calls onPressCancel when the group unmounts mid-press. Naming that case keeps a caller from assuming it never runs during its own unmount:
* The press ended with nothing acting: a cancel, a second finger,
* tracking turned off, or the group unmounting.| export {TOUCH_QUERY} from '../utils/touchQueries'; | ||
|
|
||
| // Lab's TabBar tracks its press with the shared action-group tracker. | ||
| export {useActionGroupTracking} from '../hooks/useActionGroupTracking'; |
There was a problem hiding this comment.
Non-blocking: the header's SYNC note also names scripts/touch-support-bundle.test.mjs. Its LAB_ONLY_NAMES list does not include this name yet, so the check that Lab's names stay off the root entry does not cover it:
'TOUCH_QUERY',
'useActionGroupTracking',
];# Conflicts: # packages/core/src/hooks/useActionGroupTracking.ts
|
/approve-spec 1f67124 |
There was a problem hiding this comment.
Approved pending CI: CI is still running on this head.
The hook's new paintsHighlight option and press callbacks, their re-export from Core's internal touch entry and the badged-tab string follow the action-group, internal-entry and catalog rules, and neither Core caller (AlertDialog, and ButtonGroup through useGroupPressTracking) passes the new options, so shipped behavior is unchanged. The branch now conflicts with main in packages/core/src/internal/touch.ts and its test, where #9696 appended its own section; a merge that keeps both sections resolves it, and #9700 still needs a restack onto this branch.
- Miss: (non-blocking)
onPressCancel's doc comment can also name the group unmounting mid-press, which the Effect cleanup reports as a cancel. - Miss: (non-blocking)
LAB_ONLY_NAMESinscripts/touch-support-bundle.test.mjscan listuseActionGroupTracking, as the internal entry's sync note asks.
[Automated review]
# Conflicts: # packages/core/src/internal/touch.test.ts # packages/core/src/internal/touch.ts
|
/approve-spec 4c9c002 |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #9700.
There was a problem hiding this comment.
Approved pending CI: CI is still running on this head.
The new paintsHighlight option and press callbacks extend the shared action-group tracker rather than copying it, the re-export stays on the internal entry outside the public API, and AlertDialog and ButtonGroup pass none of the new options, so they behave as before. Two earlier optional notes still apply: name unmounting in onPressCancel's doc, and add useActionGroupTracking to LAB_ONLY_NAMES in scripts/touch-support-bundle.test.mjs. #9700 does not contain this head yet; merge this branch into it to keep the stack intact.
[Automated review]
|
/approve-spec 9fc9bde |
# Conflicts: # packages/core/src/internal/touch.ts
|
/approve-spec a236965 |
There was a problem hiding this comment.
Approved pending CI: CI is still running on this head.
The new paintsHighlight option and press callbacks extend the shared action-group tracker rather than copying it, the re-export stays on the internal entry outside the public API, and AlertDialog and ButtonGroup pass none of the new options, so they behave as before. Two earlier optional notes still apply: name unmounting in onPressCancel's doc, and add useActionGroupTracking to LAB_ONLY_NAMES in scripts/touch-support-bundle.test.mjs. #9700 does not contain this head yet; merge this branch into it to keep the stack intact.
[Automated review]
# Conflicts: # packages/core/src/internal/touch.test.ts # packages/core/src/internal/touch.ts
There was a problem hiding this comment.
Approved pending CI: CI is still running on this head.
The new paintsHighlight option and press callbacks extend the shared action-group tracker rather than copying it, the re-export stays on the internal entry outside the public API, and AlertDialog and ButtonGroup pass none of the new options, so they behave as before. Two earlier optional notes still apply: name unmounting in onPressCancel's doc, and add useActionGroupTracking to LAB_ONLY_NAMES in scripts/touch-support-bundle.test.mjs. #9700 does not contain this head yet; merge this branch into it to keep the stack intact.
[Automated review]
|
/approve-spec 92659fe |
# Conflicts: # packages/core/src/internal/touch.ts
There was a problem hiding this comment.
Approved pending CI: CI has not run its tests on this head, because the CI workflow stopped at admission with "CI is full". Re-run all jobs, or push again later, so the required checks run.
The new paintsHighlight option and press callbacks extend the shared action-group tracker rather than copying it, the re-export stays on the internal entry outside the public API, and AlertDialog and ButtonGroup pass none of the new options, so they behave as before. The latest merge of main keeps both re-export sections in the internal entry. Two earlier optional notes still apply: name unmounting in onPressCancel's doc, and add useActionGroupTracking to LAB_ONLY_NAMES in scripts/touch-support-bundle.test.mjs. #9700 does not contain this head yet; merge this branch into it to keep the stack intact.
[Automated review]
|
/approve-spec 5a5ebab |
Stacked on #9754 (Core's internal touch entry and Lab's motion-engine hook). Review only this PR's change: vjeux/astryx@f3e826a...7a64fbf
Maintainer impact
Lab's TabBar lands as two Lab-only batches, drafts that follow this one. Its press needs Core's action-group tracking hook, which Lab can't reach or extend itself, and its badged tabs need one catalog string. A Lab-only batch carries no Core code, so those parts land here first, on their own.
Intended invariant
No behavior change for today's caller: the hook's new options are optional, and AlertDialog passes none. Core's internal touch entry stays internal: named re-exports only, outside the public API, and limited to Core and Lab by
@astryx/no-core-internal-import. No Core module reads the new catalog key.Change
useActionGroupTrackinggains options for a group that draws its own highlight.paintsHighlight: falsewrites no press attribute. Press callbacks report the press:onPressBeginandonPressMovewith the action under it,onPressEndbefore the lift's action runs, andonPressCancelwhen it ends with nothing acting: a cancel, a second finger, or tracking turned off mid-press.@astryxdesign/core/internal/touchre-exports the hook and its options type. TabBar's press goes through it, so a second finger ends a press with nothing chosen (family:action-groupsFR3) and focus follows the lift (FR1), as in AlertDialog.packages/core/locales/en.jsongains@astryx.tabBar.badgedItem, a badged tab's name.[chore]) names the re-export, the options and the string.Evidence
expected 'on' to be null; no callback runs, also when tracking turns off mid-press). The entry's new case fails on the current entry (useActionGroupTracking: expected undefined).Scope
Testing
Local gates at 7a64fbf, on #9754's f3e826a, node 24.22.1 from
.nvmrc. Rebased from aa755bc with no change to this PR's own lines; the one conflict, in the internal entry's export list, is resolved by adding the hook's two lines after #9754's new sections. Core typecheck; 146 unit tests (the hook's 30 cases, the entry, AlertDialog, the shared tracker, the bundle test);pnpm lint, 0 errors;review:pre. On #9700's build: AlertDialog's interaction spec, 12/12 in each engine. Red-first at the previous head: the hook's four new cases on the current hook, and the entry's new case.Stack: #9754 → #9699 → #9700
Merge group: #9699, #9700
TabBar's second Lab batch, #9701, also builds on #9700.