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.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and it needs changes either way.
No current record governs Lab TabBar or its new reselect rule (choosing the selected tab again scrolls the panel named by panelId, or its first vertically scrolling element, to the top, and still calls onReselect). The Lab exemption does not apply because the diff against main also changes Core. An engineering owner, the author included, can approve that direction by commenting /approve-spec with the full head commit SHA, or name the record that must govern it first.
Blocking findings:
- Merged with current
main, thecreateMotionAnimatortocreateMotionEnginerename leavespackages/core/src/DateInput/wheelStrip.ts(lines 19 and 135) and a new call atpackages/core/src/motion/animator.test.ts:4370importing a removed export, andpackages/lab/src/index.tsconflicts, so rebase onto the current #9754 head ormain. - The diff against
mainis 6,099 authored lines from four separable pull requests with noStack:line here, so put one identicalStack: #9754 → #9699 → #9700 → #9966line in every listed description, with each head built on the one below, or land the lower pull requests first. - TabBar turns off the shared action-group tracker's press paint (
useTabBarLens.ts:607) and repaints the same tab with its owndata-tab-bar-pressedattribute (useTabBarLens.ts:80-90), so passpaintsHighlight: !isTouch || selectedIndex < 0and style the shareddata-astryx-pressattribute instead, or extend the shared tracker and say why it cannot serve. TabBar.doc.mjs(lines 46 and 118) tells builders to use a NavigationStack that no package provides, and implies the panel scrolls only at a stack's root, while TabBar scrolls on every reselect.
The reselect scroll itself works as described in the Scroll To Top story, but CI has not run on this head yet.
[Automated review]
| * ``` | ||
| */ | ||
| export function createMotionAnimator( | ||
| export function createMotionEngine( |
There was a problem hiding this comment.
main now has packages/core/src/DateInput/wheelStrip.ts (lines 19 and 135) and a test at packages/core/src/motion/animator.test.ts:4370 that still call createMotionAnimator, so this rename breaks them once merged. Rebase onto the current #9754 head or main so the rename covers both.
There was a problem hiding this comment.
This rename is #9754's. That PR rebases onto main and renames those calls (wheelStrip.ts and the new test) before it lands, and this stack restacks onto that head.
There was a problem hiding this comment.
| // Under a mouse the host writes no touch styles and takes no selection | ||
| // lock: desktop selection is unchanged. | ||
| manageStyle: isTouch, | ||
| paintsHighlight: false, |
There was a problem hiding this comment.
This turns off the shared tracker's press paint, and paintTab (lines 80-90) then repaints the same tab with data-tab-bar-pressed. The tracker reads paintsHighlight live, so paintsHighlight: !isTouch || selectedIndex < 0 with TabBar's style keyed off [data-astryx-press="on"] would keep one press-paint path. If the shared paint cannot serve, extend it, or say why in code.
There was a problem hiding this comment.
Kept on purpose, for three reasons:
- The tracker's paint is Core's private
data-astryx-press. Lab may not write or style it (@astryx/no-raw-private-attribute,architecture:public-component-apiINV23), and strict ESLint rejects the selector in TabBar. family:action-groupsleaves how a member shows its highlight to the member. The tracking stays the shared tracker's: TabBar paints only the action the hook reports.- The tracker reads
paintsHighlighton every paint, but TabBar's choice changes with the selection and the pointer mid-press, which would strand a paint. TabBar decides once per press.
There was a problem hiding this comment.
You're right to keep it. The code already decides once per press — begin() snapshots paints: !isTouch || selected < 0, and paintTab only follows that snapshot — so a live paintsHighlight expression could change the paint mid-press when the selection or pointer changes, instead of holding the press's original decision. Given Lab may not style Core's private data-astryx-press attribute, the separate data-tab-bar-pressed paint stands. The next full review will re-evaluate the finding.
| ], | ||
| usage: { | ||
| description: | ||
| "Use TabBar to switch between an app's three to five top-level sections on a phone, each keeping its own state and navigation stack. Put a NavigationStack in each section and pop it to its root in onReselect.", |
There was a problem hiding this comment.
No package ships a NavigationStack yet, so builders following this (and the popToRoot mention on line 46) look for an import that does not exist. Refer to the app's own navigation stack until it lands.
There was a problem hiding this comment.
Fixed for the next push. The text now says "return the tab's navigation stack to its root" without naming a component, and that TabBar scrolls the tab's panel to its top on every reselect.
26fb252 to
de5fe68
Compare
|
Restacked onto #9700's new head. The docs now say "return the tab's navigation stack to its root" without naming a component, and that TabBar scrolls the tab's panel to its top on every reselect. |
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
/approve-spec de5fe68 |
There was a problem hiding this comment.
Changes are needed in #9754, and #9700 and #9966 wait on an engineering owner's decision.
#9754: changes requested
.changeset/core-internal-touch-entry-lab-animator.mdsays Lab's PageControl and NavigationStack read the new internal-entry names, but no package ships either component, so name only TabBar (or "Lab components") in that release note.
#9699: can be approved on its own.
#9700: waiting on an owner decision
- No current record governs Lab
TabBarandTabBarItem, and the stack also changes Core and the Storybook app, so the Lab exemption does not apply: an engineering owner approves this direction by commenting/approve-specwith the full head SHA, or names the record that must govern it, and the author may do so if they are an engineering owner.
#9966: waiting on the same decision
- No current record governs the reselect rule (choosing the selected tab again scrolls the panel
panelIdnames, or its first vertically scrolling element, to the top, and still callsonReselect), and the same/approve-speccovers it.
Whole stack: changes requested, because of the #9754 changeset. A pull request lands only when it and every pull request below it are approved, and CI has not run on this head yet.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: Core's internal motion engine factory is renamed `createMotionEngine` (from `createMotionAnimator`), with its types `MotionEngine` and `MotionEngineOptions`, at its source and in every internal caller. Core's internal touch entry also re-exports, by name, the UIKit gesture values, the right-to-left check, and the touch-presentation hook and query that Lab's PageControl, NavigationStack and TabBar read. Lab's `useMotionEngine` hook wraps the engine, and Lab's refresh control uses it. Apps import nothing new. |
There was a problem hiding this comment.
No package ships a Lab PageControl or NavigationStack in this stack (or on main), so Core's release notes would announce readers that do not exist. Name only TabBar here (or say "Lab components"); the comment at packages/core/src/internal/touch.ts:77 has the same names.
There was a problem hiding this comment.
I'll run a full review to re-check this with your note.
Internal entries land with their first user, per meta#8562. Lab components, TabBar first, animate with Core's motion engine and read the touch presentation and the writing direction as Core does. `@astryxdesign/core/internal/touch`, which already carries the pan recognizer, its hook, the engine and the rubber band for Lab's refresh control (meta#9807), gains by name: - `useTouchPresentation` and `TOUCH_QUERY`, the finger-or-mouse decision in render and outside it; - `isRtlElement`, the writing direction a pan mirrors by; - UIKit's `UIKIT_GESTURES` table. The engine's factory is renamed from `createMotionAnimator` to `createMotionEngine` at its source and in every internal caller, with its types `MotionEngine` and `MotionEngineOptions` (the engine's `Animator` and `AnimatorOptions`), so the engine and the public `createAnimator` differ by more than one word (architecture:react-component-runtime INV13). Named re-exports of code already on main, plus that rename: no behavior changes, no public docs, no root re-export, and the `@astryx/no-core-internal-import` lint rule already limits the entry to Core and Lab. The entry's tests check it re-exports the same values Core imports by path and carries no public factory's name; the touch-support bundle test keeps every Lab-only name out of the root entry. One Lab module those components share comes with them: `internal/useMotionEngine`, a stable motion engine bound to an element through a callback ref. Lab's refresh control now uses it in place of its own identical `RefreshControl/useMotionAnimator.ts`.
…tracking hook, extended for Lab's TabBar, and a tab-bar string joins the catalog Re-exports only on the entry: @astryxdesign/core/internal/touch adds useActionGroupTracking and its options type. So Lab's TabBar presses through the hook AlertDialog uses: a second finger ends the press with nothing acting, and focus follows the lift. The hook gains options for a group that draws its own highlight. All are optional, and without them the hook runs as before, so AlertDialog does not change. A no-paint option leaves the highlight to the group. Press callbacks report the action under the press as it begins and after each move, the lift before its action runs, and a cancel. Core's message catalog gains TabBar's badged-tab name. It lands ahead of the Lab component, so the Lab batch changes only Lab. The entry stays outside the public API: no docs, no root re-export, and the @astryx/no-core-internal-import rule keeps it to Core and Lab.
… and press tracking TabBar shows an app's top-level sections as iOS 26's floating capsule above the bottom edge, with a selection lens that slides to the chosen tab. Each TabBarItem has an icon, a label and an optional badge. Under a finger, on iOS and Android alike, the lens moves on the springs measured from UIKit on iOS 26.2: it lifts into glass as a finger lands, follows a finger dragged across the tabs and settles on the tab nearest the lift, while the bar flexes. A pen in the touch presentation does the same. Under a fine pointer a press is a plain button press: a click chooses a tab and the lens follows the theme's motion. Sliding off the bar cancels: a press lifted off it chooses nothing. Choosing the selected tab again is reported apart, so a screen can return to its root. Core's action-group tracking owns the press, in the touch presentation only: the tab under it, running that tab at the lift, a second finger's cancel and focus after the lift. TabBar draws the lens. With no tab selected there is no lens, so a held finger paints the tab under it. A tab names the panel it controls through panelId, rendered as aria-controls. TabBar's UIKit values live beside it in uikitTabBar.ts, each with its source, and uikitTabBar.test.ts checks that every value names one. Core's gesture and motion pieces come through Core's internal touch entry.
…o the top As on iOS, choosing the selected tab again brings its section back to the top. TabBar finds the tab's panel through panelId and scrolls that panel's own scroll area: the panel itself when it scrolls, or else the first scroll area inside it. Nothing else scrolls: not the page, not the other panels, not any other scroll area. The panel glides to the top, and under Reduce Motion it jumps. onReselect is still called, so an app can also return the tab's navigation stack to its root.
de5fe68 to
372e68f
Compare
|
/approve-spec 372e68f |
There was a problem hiding this comment.
This stack waits on an engineering owner's decision; no code changes are needed.
#9754: can be approved on its own. The internal rename and re-exports stay on Core's internal entry, and the release note now names no component the stack does not ship.
#9699: can be approved on its own. The tracker's new options are optional, and AlertDialog's path is unchanged.
#9700: waits on an engineering owner's decision.
- No current spec record governs Lab
TabBarandTabBarItem(the selection lens, press tracking and badges), so an engineering owner needs to approve them as Lab direction.
#9966: waits on an engineering owner's decision.
- No current spec record governs the reselect rule: choosing the selected tab again scrolls the panel its
panelIdnames (the panel itself if it scrolls vertically, else the first vertically scrolling element inside it) to the top, smoothly or as a jump under Reduce Motion, and still callsonReselect.
Whole stack: the cumulative change has no blocking issue. A pull request lands only when it and every pull request below it are approved.
This direction was approved at the stack's previous head, but the restack reset that approval. An engineering owner records it again by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. CI has not run on this head yet.
[Automated review]
| /** The items' shared width (and the resting lens's), inside a calc(). */ | ||
| const size = (count: number) => itemWidthCss(count); |
There was a problem hiding this comment.
Non-blocking: size only renames itemWidthCss, and its comment restates the type. Call itemWidthCss(count) directly and drop these two lines.
There was a problem hiding this comment.
Leaving this for the next TabBar change. Every head in this stack has its owner approval at its exact commit, and a push now would reset those for a non-blocking rename.
There was a problem hiding this comment.
I'll run a full review to re-check this with your note.
|
Specs & Decisions agent for Cindy: approved under the Lab grant and Cindy's tap-to-top go: choosing the selected tab again scrolls the panel its panelId names (the panel itself if it scrolls vertically, else the first vertically scrolling element inside it) to the top, smoothly, or as a jump under Reduce Motion, and still calls onReselect. (Core changes in this diff belong to #9754 and #9699 beneath it, approved separately.) /approve-spec 372e68f |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once these drafts are marked ready for review and a maintainer adds the ci:run label.
#9754: approve. No blocking findings.
#9699: approve. No blocking findings.
#9700: approve. No blocking findings; an engineering owner approved Lab TabBar and TabBarItem as Lab direction at this pull request's head.
#9966: approve. No blocking findings; an engineering owner approved the reselect-to-top rule at this head, and it behaves as approved.
Whole stack: no blocking issue. A pull request lands only when it and every pull request below it are approved.
[Automated review]
cixzhang
added
the
ci:run
There was a problem hiding this comment.
This stack waits on an engineering owner's decision; no code changes are needed.
#9754: can be approved on its own. No blocking findings.
#9699: can be approved on its own. No blocking findings.
#9700: waits on an engineering owner's decision.
- No current spec record governs Lab
TabBarandTabBarItem(the selection lens, press tracking and badges), so an engineering owner needs to approve them as Lab direction at this pull request's current head.
#9966: waits on an engineering owner's decision.
- No current spec record governs the reselect rule: choosing the selected tab again scrolls the panel its
panelIdnames (the panel itself if it scrolls vertically, else the first vertically scrolling element inside it) to the top, smoothly or as a jump under Reduce Motion, and still callsonReselect.
Whole stack: no blocking issue. A pull request lands only when it and every pull request below it are approved.
Merging main into each pull request made new heads, and an approval counts only at its exact head. An engineering owner records the direction again by commenting /approve-spec <full head SHA> on #9700 and on this pull request; the author may do so as an engineering owner.
[Automated review]
|
Specs & Decisions agent for Cindy: approved under the Lab grant and Cindy's tap-to-top go: choosing the selected tab again scrolls the panel its panelId names (the panel itself if it scrolls vertically, else the first vertically scrolling element inside it) to the top, smoothly, or as a jump under Reduce Motion, and still calls onReselect. (Core changes in this diff belong to #9754 and #9699 beneath it, approved separately.) /approve-spec 5af8812 |
There was a problem hiding this comment.
Changes are needed in #9966; #9754, #9699 and #9700 can be approved.
#9754: approve. No blocking findings.
#9699: approve. No blocking findings.
#9700: approve. No blocking findings; an engineering owner approved Lab TabBar and TabBarItem as Lab direction at this pull request's current head.
#9966: changes requested.
- Miss:
panelScrollAreainpackages/lab/src/TabBar/panelScroll.ts(lines 29-34) tests for a scroller with its ownscrollHeight > clientHeightinstead of Core's shared effective-scroll test inpackages/core/src/hooks/scrollGeometry.ts, which needs content more than 1 px taller than the box, so a sideways chip row whose chips are 1 px taller than the row is taken and choosing Home again leaves Home's list where it was; re-export Core's test through the internal entry and call it, with a unit case for that row, or say in the code why it cannot serve.
Whole stack: no further blocking issue. A pull request lands only when it and every pull request below it are approved.
[Automated review]
| (overflowY === 'auto' || overflowY === 'scroll') && | ||
| element.scrollHeight > element.clientHeight |
There was a problem hiding this comment.
Miss: this re-implements Core's effective-scroll test (measureLogicalScrollAxis in packages/core/src/hooks/scrollGeometry.ts, which needs content more than 1 px taller than the box and also accepts overlay). With no tolerance, a sideways chip row whose chips are 1 px taller than the row (48 px of content in a 47 px box) is taken here; it sits at scrollTop 0, so scrollPanelToTop returns and the list below keeps its offset. Re-export Core's test by name through the internal entry, as this stack does for isRtlElement, call it for each candidate, and add a unit case for that row; or say here why Core's test cannot serve.
There was a problem hiding this comment.
Fixed: panelScrollArea now asks Core's measureLogicalScrollAxis for the vertical axis, so a row that overflows by a pixel or less is skipped. Core's internal touch entry re-exports it by name, with its entry test, the bundle test's Lab-only list and a [chore] changeset. A new unit case covers the 48 px row in a 47 px box; it fails without the change.
Choosing the selected tab again scrolls the tab's panel to its top. TabBar took the first element whose content was taller than its box, so a chip row 1 px taller than its content box was taken, and the list below never moved. It now asks Core's shared scroll test, which needs more than 1 px of overflow and also accepts overlay scrolling. Core's internal touch entry re-exports that test by name for it.
|
Specs & Decisions agent for Cindy: approved under the Lab grant and Cindy's tap-to-top go: choosing the selected tab again scrolls the panel its panelId names (the panel itself if it scrolls vertically, else the first vertically scrolling element inside it) to the top, smoothly, or as a jump under Reduce Motion, and still calls onReselect. (Core changes in this diff belong to #9754 and #9699 beneath it, approved separately.) This head also adds an internal-only Core re-export of measureLogicalScrollAxis for Lab TabBar (no public API change). /approve-spec 76a2f2a |
There was a problem hiding this comment.
This pull request waits on an engineering owner's decision, and it needs one change either way.
- No current spec record governs Lab
TabBarandTabBarItemor the reselect rule: choosing the selected tab again scrolls the panel itspanelIdnames (or the first vertically scrolling element inside it) to the top, smoothly or as a jump under Reduce Motion, and still callsonReselect, and the earlier/approve-specon this pull request names the head before the commit that switched the lookup to Core's scroll test, so it no longer counts. - Regression: the description now declares
Stack: #9966while #9754, #9699 and #9700 declareStack: #9754 → #9699 → #9700, so this pull request is reviewed on its own, and its 6,094 authored lines, 5,597 of them from those three pull requests, get no stack exemption; restoreStack: #9754 → #9699 → #9700 → #9966in all four descriptions, or update this branch frommainafter the three land.
The earlier panelScroll.ts finding is fixed: the lookup now uses Core's scroll test, and a chip row 1 px taller than its box is skipped. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. CI has not run on this head yet because the pull request is a draft.
[Automated review]
There was a problem hiding this comment.
This pull request needs one change before it can be approved. The reselect-to-top behavior itself is approved and has no code defect.
- Regression: the description declares
Stack: #9966, while #9699 and #9700 declareStack: #9754 → #9699 → #9700, so this branch is reviewed on its own and its 6,094 authored lines, 5,571 of them from #9754 (already merged), #9699 and #9700, get no stack exemption; update the branch frommain(resolving the conflicts inpackages/core/src/internal/touch.ts,packages/core/src/internal/touch.test.tsandscripts/touch-support-bundle.test.mjs), restack it on #9700's current head and add one identicalStack: #9699 → #9700 → #9966line to all three descriptions, or update it frommainafter #9699 and #9700 land.
The engineering owner's /approve-spec for this head covers Lab TabBar, TabBarItem and the reselect rule; a new commit needs a fresh /approve-spec for its own head. One non-blocking cleanup remains: in TabBar.tsx, size only renames itemWidthCss.
[Automated review]
Stacked on #9700 (on #9699, on #9754). Review only this PR's change: vjeux/astryx@7755e54...76a2f2a
User need
On iOS, tapping the tab you are already on brings that section back to its top. TabBar reports the reselect (
onReselect), but each app then has to find the section's scroll area and animate it, and a wrong guess scrolls the page or another region.Problem and solution fit
Choosing the selected tab again, by a tap, a click or a key, now scrolls that tab's own scroll area to its top. TabBar finds the panel through the tab's
panelIdand scrolls the panel itself when it scrolls vertically, or else the first element inside it that does. Core's shared scroll test decides, so a row that overflows by a pixel or less, or only sideways, is skipped. Nothing else scrolls: not the page, not the other panels, not any other scroll area. The area glides to the top; under Reduce Motion it jumps.onReselectis still called, so an app can also return the tab's navigation stack to its root.Current specification
Public delta and caller burden
No new props. A tab that names its panel (
panelId) now scrolls that panel on reselect; a tab without one behaves as before. A caller puts each section's content in a ScrollableArea inside its panel, or makes the panel the scroll area:Design and alternatives
smooth, or an instant jump under Reduce Motion).measureLogicalScrollAxis, as ScrollableArea uses), which Core's internal touch entry re-exports by name; apps import nothing new.panelIdalready names the panel foraria-controls), and scrolling the document (it would move scrollers outside the tab).Evidence
The new Scroll To Top story: each panel has a sideways row of chips over its list. Scroll Home down, then choose Home again: Home's list glides to its top, and the pinned scroller above stays put.
Scope
Testing
Local gates at 76a2f2a (on #9700 7755e54), node 24.22.1 from
.nvmrc:overflow-y: autoelement (3f2556f) the chips row was taken and the same cases stayed at 800. Choosing another tab scrolled nothing on every build.pnpm lint, 0 errors; Prettier;check:changesets, and Changeset coverage for this PR alone over feat(lab): TabBar, iOS 26's floating tab bar, with its selection lens and press tracking #9700;review:pre.Stack: #9966