Repository navigation
refactor(Resizable): the resize model's pure parts live in one non-React module - #9197
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| ResizeHandle | 25.7 kB (−1 B) · 4.0 kB CSS (0) | — | ✓ · ✓ · ✗ |
| resizable | — | — | — · ✓ · ✗ |
| Resizable | — | — | ✓ · ✓ · ✗ |
1 more component
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| SideNav | 67.8 kB (−1 B) · 10.9 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 5.66 MB packed (+1.2 kB); no new dependencies.
API changes: core +8 · ~20 · −0 (report stale: run pnpm api:report); richtext +0 · ~1 · −0 (report stale: run pnpm api:report)
29 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Types · interface FieldStatusProps |
message: string |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {message: …} |
| + | core |
. · Types · interface FieldStatusProps |
ref?: React.Ref<HTMLDivElement> |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {ref: …} |
| + | core |
. · Types · interface FieldStatusProps |
type: InputStatusType |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {type: …} |
| + | core |
. · Types · interface FieldStatusProps |
variant?: FieldStatusVariant = 'attached' |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {variant: …} |
| + | core |
. · Types · interface FieldStatusProps |
xstyle?: StyleXStyles |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {xstyle: …} |
| + | core |
. · Types · interface FieldStatusProps |
inherits AriaAttributes, DOMAttributes, HTMLAttributes |
import type {FieldStatusProps} from '@astryxdesign/core' |
| + | core |
. · Types · interface SearchContext |
interface SearchContext<T extends SearchableItem = SearchableItem> |
import type {SearchContext} from '@astryxdesign/core' |
| + | core |
./Typeahead · Also exported from an earlier entry point · SearchContext |
SearchContext (.) |
import {SearchContext} from '@astryxdesign/core/Typeahead' |
| ~ | core |
. · Components · component ComplexSelector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary and ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary and ghost selectors |
<ComplexSelector statusVariant="attached" /> |
| ~ | core |
. · Components · component DateInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<DateInput statusVariant="attached" /> |
| ~ | core |
. · Components · component DateRangeInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<DateRangeInput statusVariant="attached" /> |
| ~ | core |
. · Components · component Field |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Field statusVariant="attached" /> |
| ~ | core |
. · Components · component FieldStatus |
component FieldStatus(props: FieldStatusProps) → component FieldStatus @deprecated |
<FieldStatus /> |
| ~ | core |
. · Components · component FieldStatus |
prop variant?: FieldStatusVariant = 'attached' → prop variant: FieldStatusVariant = 'attached' |
<FieldStatus variant="attached" /> |
| ~ | core |
. · Components · component FileInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<FileInput statusVariant="attached" /> |
| ~ | core |
. · Components · component MultiSelector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary and ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary and ghost selectors |
<MultiSelector statusVariant="attached" /> |
| ~ | core |
. · Components · component NumberInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<NumberInput statusVariant="attached" /> |
| ~ | core |
. · Components · component PowerSearch |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<PowerSearch statusVariant="attached" /> |
| ~ | core |
. · Components · component Selector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary/ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary/ghost selectors |
<Selector statusVariant="attached" /> |
| ~ | core |
. · Components · component TextArea |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TextArea statusVariant="attached" /> |
| ~ | core |
. · Components · component TextInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TextInput statusVariant="attached" /> |
| ~ | core |
. · Components · component TimeInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TimeInput statusVariant="attached" /> |
| ~ | core |
. · Components · component Tokenizer |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Tokenizer statusVariant="attached" /> |
| ~ | core |
. · Components · component Typeahead |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Typeahead statusVariant="attached" /> |
| ~ | core |
. · Types · interface FieldStatusProps |
interface FieldStatusProps extends BaseProps<HTMLDivElement> (props of FieldStatus) → interface FieldStatusProps extends BaseProps<HTMLDivElement> |
import type {FieldStatusProps} from '@astryxdesign/core' |
| ~ | core |
. · Types · interface FieldStatusVariantMap |
tooltip: true → tooltip: true @deprecated |
import type {FieldStatusVariantMap} from '@astryxdesign/core'; const x: FieldStatusVariantMap = {tooltip: …} |
| ~ | core |
. · Types · interface SearchSource |
search: (query: string) => Promise<T[]> | T[] → search: (query: string, context?: SearchContext<T>) => Promise<T[]> | T[] |
import type {SearchSource} from '@astryxdesign/core'; const x: SearchSource = {search: …} |
| ~ | core |
. · Types · interface UseInputStatusIconOptions |
statusVariant?: FieldStatusVariant → statusVariant?: FieldStatusPlacement |
import type {UseInputStatusIconOptions} from '@astryxdesign/core'; const x: UseInputStatusIconOptions = {statusVariant: …} |
| ~ | richtext |
. · Components · component RichTextEditor |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusVariant | 'tooltip' = 'attached' |
<RichTextEditor statusVariant="tooltip" /> |
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Current run: CI concluded failure; parts of this report may be missing.
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.
Approval needs an engineering owner's spec decision, and the change also needs work. component:Resizable FR3 now says createResizable owns the behavior that ResizeHandle and useResizable go through; an engineering owner records approval, or asks for narrower wording, by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- The docs site build stops on the ShadCN route lock: the new
createResizabledoc page addshooks/create-resizable, so the lock needs refreshing withUPDATE_SHADCN_ROUTE_LOCK=1. - The configuration-object
useResizableandcreateResizableeach keep their own copy of the region state (bounds, collapse, saving); building one on the other would also make FR3's new sentence true. architecture:public-component-apiINV22 deprecates the configuration-object form when the ref form arrives, rather than in a later patch.- With a plain handle element,
useResizable(handleRef)announces "Resize handle" and "Collapsed" untranslated and keeps its listeners on a handle that has unmounted. - At 4,883 changed lines the change needs splitting; landing #7782 first and moving the model extraction to its own pull request are clean cuts.
[Automated review]
| // Copyright (c) Meta Platforms, Inc. and affiliates. | ||
|
|
||
| /** @type {import('@astryxdesign/cli/authoring').HookDoc} */ | ||
| export const docs = { |
There was a problem hiding this comment.
This page adds the ShadCN registry item hook-create-resizable at hooks/create-resizable, which internal/shadcn-registry/routes.lock.json doesn't list, so apps/docsite/scripts/generate-data.mjs stops with "Generated ShadCN names or routes changed" and the docs site doesn't build. Refreshing the lock with UPDATE_SHADCN_ROUTE_LOCK=1 node apps/docsite/scripts/generate-data.mjs adds just that entry.
| | --- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------- | | ||
| | FR1 | ResizeHandle renders one focusable Handle and one enlarged Grab zone (a `--spacing-4` strip on the resize axis); the default render also contains a Grip pill, and caller `children` replace the pill. | Current source, docs, and tests | | ||
| | FR2 | The Handle carries `resize-handle`, the Grip pill carries `resize-handle-pill`, and the Grab zone carries no public target. | Current source and docs | | ||
| | FR3 | Pointer drag, keyboard, double-click collapse, snapping, persistence, reversal, and size behavior are owned by `createResizable`, which ResizeHandle and `useResizable` go through; dragging under right-to-left inverts the pointer delta so the divider follows the pointer. A disabled Handle ignores pointer and keyboard input and exposes `aria-disabled`, and keeps its tab stop (`spec:AST-085` FR1). With `autoSaveId`, storage holds the last settled size and receives the final size once when a drag ends, cancels, or unmounts. Implementation gap: a disabled Handle sets `tabIndex={-1}` (Current-state impact). | Current source, docs, and tests | |
There was a problem hiding this comment.
At this head the configuration-object useResizable({...}) and a ResizeHandle given a resizable prop don't call createResizable; they share only resizeGesture.ts and resizableModel.ts. Either build that form on the factory or word this row so it names the ref form only.
| } | ||
|
|
||
| /** The region's state from its options, its basis and its selection. */ | ||
| function compute(): void { |
There was a problem hiding this comment.
This and the persistence, collapse and gesture code below repeat useSingleResizable in useResizable.ts (default latching, bound re-clamp, save once per gesture, collapse/expand/resize). Could the configuration-object form drive createResizable(null, …) so the region state lives in one place?
| import type {ResizableSize} from './utils'; | ||
|
|
||
| /** The English names the handle carries when no other is given. */ | ||
| const DEFAULT_LABEL = 'Resize handle'; |
There was a problem hiding this comment.
These English defaults become aria-label and aria-valuetext on a plain handle, including through useResizable(handleRef), which passes no translated value. ResizeHandle resolves @astryx.resizable.handle.label and @astryx.resizable.collapsed through the translator; the ref hook could pass those as defaults.
| useIsomorphicLayoutEffect(() => { | ||
| const element = ref.current; | ||
| const entry = slot.get(); | ||
| if (entry != null && (element == null || entry.element === element)) { |
There was a problem hiding this comment.
When the handle unmounts (element == null) the old factory stays attached to the detached node, so a plain handle keeps its listeners and attributes until a new node appears or the hook unmounts. Destroying it here and continuing with a handle-less region (or a factory method that moves to a new element or to none) would release the node.
10af9d4 to
59ce878
Compare
vjeux
changed the title
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and the change also needs work. component:Resizable FR3 now says createResizable owns the behavior that ResizeHandle and useResizable go through; an engineering owner approves that clause, or asks for narrower wording, by commenting /approve-spec 59ce878798f3de6f471db584bb732c2a405f6b58 on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- The configuration-object
useResizableandcreateResizablekeep two copies of the region state and of the slide's measure-and-land logic; building that form on the factory would also make FR3's clause true. - The configuration-object hook now returns
isCollapsedandsizeone commit afteronCollapseChangefor every caller, SideNav included, while FR8 says they stay unchanged. LayoutPanelhands its pane to the region in a layout effect that itslendRefcallback ref can do.- With a plain handle element,
useResizable(handleRef)announces "Resize handle" and "Collapsed" untranslated and keeps its listeners on a handle that has unmounted. architecture:public-component-apiINV22 deprecates the configuration-object form when the ref form arrives, rather than in a later patch.- FR8's browser verification covers
collapse()and Enter only; the drag past the threshold, the reopening drag, the grow arrow, a controlled change and a hook-only pane need frames too. - At 9,232 changed lines the change needs splitting: landing #8721 first, moving the model extraction out, and putting the slide in its own pull request are clean cuts.
[Automated review]
| useIsomorphicLayoutEffect(() => { | ||
| if (shownIsCollapsed === isCollapsed) { | ||
| return; | ||
| } | ||
| regionPane.beforeCollapseChange(isCollapsed); | ||
| // eslint-disable-next-line @eslint-react/set-state-in-effect -- the new collapse state renders one synchronous commit after the measurement above, which needs the pane still at its old size; both commits land before paint | ||
| setShownIsCollapsed(isCollapsed); | ||
| }, [isCollapsed, shownIsCollapsed, regionPane]); |
There was a problem hiding this comment.
This trailing commit runs for every configuration-object region, SideNav's included, whether or not a LayoutPanel lent it a pane. After collapse() a caller's onCollapseChange state is collapsed while the hook still returns isCollapsed=false and size=200 for one commit, and a passive effect sees that mix; main renders once. Measuring in collapse/expand/resize/_onResizeMove before they set state, and trailing only a controlled isCollapsed change on a region with a lent pane, would keep FR8's "size, isCollapsed … unchanged".
| useIsomorphicLayoutEffect(() => { | ||
| if (regionPane == null) { | ||
| return; | ||
| } | ||
| regionPane.attachPane(paneLink.pane()); | ||
| return () => { | ||
| regionPane.attachPane(null); | ||
| }; | ||
| }, [regionPane, paneLink]); |
There was a problem hiding this comment.
This hands the lent pane to the hook's region when the element attaches, which lendRef above already observes. Calling regionPane?.attachPane(...) from lendRef (with regionPane in its dependencies) would let this Effect go.
59ce878 to
ee2328a
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and the change also needs work. component:Resizable FR3 now says createResizable owns the behavior that ResizeHandle and useResizable go through; an engineering owner approves that clause, or asks for narrower wording, by commenting /approve-spec ee2328ac6afff2c1d96507794979285256f301c2 on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- The configuration-object
useResizableandcreateResizablekeep two copies of the region state and of the slide's measure-and-land logic; building that form on the factory would also make FR3's clause true. - The configuration-object hook now returns
isCollapsedandsizeone commit afteronCollapseChangefor every caller, SideNav included, while FR8 says they stay unchanged. LayoutPanelhands its pane to the region in a layout effect that itslendRefcallback ref can do.- With a plain handle element,
useResizable(handleRef)announces "Resize handle" and "Collapsed" untranslated and keeps its listeners on a handle that has unmounted. - INV22 deprecates the configuration-object form when the ref form arrives, rather than in a later patch.
- The committed browser spec covers only
collapse()and Enter; FR8's verification also asks for the drag paths, double-click, grow arrow, a controlled change and a hook-only pane, and the two records still list the slide and focus move as gaps. - At 9,624 changed lines the change needs splitting: landing #8721 first, moving the model extraction out, and putting the slide in its own pull request are clean cuts.
[Automated review]
…ch the SideNav rail moves onto (#8721) Implements the records #8783 put on main: `spec:AST-105` FR2 and FR9, `architecture:react-component-runtime` INV12, `component:SideNav` FR6, `component:Resizable` FR8, `component:LayoutPanel` FR7 and AR2, and `component:Table` FR15. Resizable's and LayoutPanel's slide and LayoutPanel's focus rule (AR2: focus moves to the ResizeHandle only when a collapsing pane holds it) are built on this path in #9197 and #9202. This PR moves no focus anywhere. Behavior-module PR: `packages/core/src/motion/animatedLayout.ts` (FR9's shared edge reveal). ## User need A panel that collapses beside content, a Collapsible closed while it opens, a resizable pane sliding shut: each reveals or conceals an element from one of its edges while the content beside it moves. Before this change Core had one such reveal of its own per component. The SideNav rail clipped and rode its content in its own code. Collapsible's reveal could only open downward, and a close while it opened restarted the conceal from fully shown while the content below kept gliding, so a gap opened. Nothing a resize controller could reuse existed. #8783 made these one shared edge reveal in the animated-layout module. ## Problem and solution fit The animated-layout module now produces `spec:AST-105` FR9's edge reveal, in three forms over one ride: - **One-state** (`playLayoutChange`, `holdLayoutExit`): reveals and conceals from the block-end edge by default, or from the block-start or an inline edge it is given, mirrored right to left. Once the content on that edge's side glides, the moving edge rides that glide frame by frame (its keyframes replaced, its clock the glide's), so no gap opens. It rides the content's offset relative to the element: when the same batch also moves the element itself, such as an item above them closing in a single-open CollapsibleGroup, the edge follows only the difference. A conceal by a start edge frees the element's space on that edge's side, so the element stays where it shows while the content before it moves; a layout where that would move the element itself is not held, and the element goes at once. A reversal starts from what the element shows. Collapsible, TreeList and Table's row expansion already play this block-end form, so they gain the ride, and Collapsible and TreeList, which hold their exits, the reversal too. - **Two-state** (`holdLayoutEdge`, `playLayoutEdge`): holds the old state as FR8's swap copy, clips the larger state on the edge from the extent it shows, rides a gliding element, and raises the element above the copy while it conceals. The SideNav rail now plays through it on its inline-end edge and keeps only its labels' and icons' fades (`component:SideNav` FR6); its frames are unchanged. - **A resized element with no copy** (`reportLayoutEdge`): measured before a size change commits and played after the new size applies. A larger extent reveals from the extent shown. A smaller one holds the element at the extent it was laid out at, freeing the difference by the margin on that edge's side so what lies beyond stays where the new size puts it, then clips down to the new extent. This is the path a resize controller's pane uses (`component:Resizable` FR8 and `component:LayoutPanel` FR7 in #9197 and #9202). Reduce Motion follows `design:reduced-motion` DR2 under a coarse primary pointer and DR3 otherwise. ## Current specification `spec:AST-105` FR9 ("One shared edge reveal") and FR2, `architecture:react-component-runtime` INV12, `component:SideNav` FR6, `component:Resizable` FR8, `component:LayoutPanel` FR7 and AR2, and `component:Table` FR15, all current on main since #8783. A LayoutPanel given a hand-built `ResizableProps` gets no slide, and the hook's own `props` passed through `resizable` slide; both are #9197 and #9202's to implement. ## Public delta and caller burden None. The new functions and types stay internal: no entry point exports them (`animatedLayout.boundary.test.ts` checks this), and `useAnimatedLayout`'s contract is unchanged. The visible changes are Collapsible's and TreeList's: content closed while it opens, or opened while it closes, continues from what it shows, and its edge stays on the content after it (`[fix]` changeset). Table's row expansion reveal rides the rows after it. ## Design and alternatives - **Riding:** the edge's keyframes are sampled on the glide's own sample grid and clock, so the clip and the glide are complementary by construction. The alternative, a separate spring tuned to match, drifts whenever the glide carries a running motion. - **Resized element without a copy:** holding the old extent keeps the pane's content laid out as it was. Clipping a pane already at its new size would need its content to overflow, or a copy, which the runtime record rules out for a resize controller. - **Morph routing** of the glide stays out until the shared rect morph lands (#8329), as INV12's "until then" clause says. ## Evidence Status at this head: rebased onto main 8a03d75 after the component records moved to `docs/specs/components/`; the patch is byte-identical to 70ebcc7's. Here, build, type check, CI-strict lint, `check:knowledge`, the record-path tests (knowledge paths, check-knowledge, specs-for-file), `check:changesets` and the motion, SideNav and AppShell unit suites (1,020) pass, and `specs:for` still returns `component:SideNav` and `spec:AST-105` for the changed files. At 70ebcc7 every gate finished: `test:ui`, `check:repo`, the node suites (only the two `cache.test.mjs` lock cases that fail on main), and the three motion specs ×10 in WebKit under the CI job's command (740, with 100 Chromium-only skips) and in Chromium pinned to 2 cores at 4 workers (840). **Still running:** `test:ui`, the node suites, `check:repo` and the three motion specs ×10 in both engines at this head. Each new proof failed on main first: - `animatedLayout.edge.test.ts`: 17 of the 18 one- and two-state cases fail on main (the block-end default passes there and guards it), and the resized-element cases have no function there. - `SideNavRailMotion.test.tsx`: a new case asserts the rail plays through the module's two-state form with no clip, copy or ride of its own. It fails on main; the rail's existing cases pass unchanged with the rail moved. - `animatedLayout.interaction.browser.spec.ts` (FR9): a block-end reveal reversed into a conceal, an inline-end reveal and conceal (LTR and RTL), the two-state form, and a resized pane (start pane LTR to 48 px, start pane RTL to 0, end pane LTR to 48), each with a reversal. On every frame the moving edge stays within 1.5 px of what it rides. All fail on main: the block-end reversal jumped 40 px, the inline edge was clipped from the bottom, and the other forms don't exist there. - `animatedLayoutAdopters.interaction.browser.spec.ts`: a Collapsible closed while it opens, and a TreeList row collapsed while it expands, keep their edge on the content after them. On main each conceal restarted from fully shown: −16 px and −36 px gaps. Two more cases guard this PR's own paths. A block-start reveal and conceal in a column that stacks from its end keep the edge on the content before it, with the element staying where it shows; before the start-edge fix the conceal freed its space at its end, so the element jumped by its 80 px and an 80 px gap opened. And in a single-open CollapsibleGroup switching from a taller item to a shorter one below it, the opening item's edge stays on the next item on every frame; before the ride was taken relative to the element, the content showed 31 px over the next item. How the proofs read a ride: both sides on the module's clock, the edge from its clip animation's keyframes and what it rides from its layout position plus its glide's keyframes, at the animations' current time. Chromium also asserts that its evaluated clip and rect equal those (under 0.5 px). In WebKit the two run a little apart, and that skew is reported per test, not hidden: a compositor-run `translate` is evaluated a little ahead of the frame and, on a late first frame, a main-thread clip reads one frame behind. Under the CI WebKit job's command (`LP_NUM_THREADS=4`), the summed skew per frame reached 0.7 to 3.4 px depending on the test. The reversals pause and seek every animation to 120 ms, and the adopters' wait for the component to commit the close before playing on, so neither a late first frame nor a component committing in a later task moves the moment they test. ## Scope - [x] One capability is added (FR9's shared edge reveal), with its adopters in this PR's scope (the SideNav rail; Collapsible, TreeList and Table's row expansion keeping their form). - [x] Unrelated fixes, cleanup, and policy changes were removed or split. Resizable's and LayoutPanel's adoption is #9197 and #9202. - [x] Consumer docs, focused tests, and migration evidence ship with the feature. - [x] Public text and artifacts contain no internal Meta context. Overlaps: #8677 also edits `motion/animatedLayout.ts` (the theme's motion knobs), and #3890 edits `SideNav/SideNav.tsx`. This PR leaves `SideNav.tsx` unchanged. ## Testing ``` pnpm exec vitest run --project ui packages/core/src/motion packages/core/src/SideNav packages/core/src/AppShell packages/core/src/Collapsible packages/core/src/TreeList packages/core/src/Table pnpm build && pnpm storybook:build pnpm test:interaction packages/core/src/motion/__tests__/animatedLayout.interaction.browser.spec.ts packages/core/src/motion/__tests__/animatedLayoutAdopters.interaction.browser.spec.ts packages/core/src/SideNav/__tests__/SideNavRailMotion.interaction.browser.spec.ts --project chromium ``` For WebKit, use the CI WebKit job's container command (`internal/interaction-harness/scripts/webkit-container.mjs`) with podman `--cpus=4`, `CI=1`, `LP_NUM_THREADS=4` and 1 worker.
…act module Size parsing and resolution against a basis, clamping and snapping, persistence and the content-box measurement move from useResizable.ts to resizableModel.ts unchanged, so the factory that comes next resolves, clamps, snaps and persists exactly as the hook does. No behavior change.
ee2328a to
57eccdd
Compare
github-actions
Bot
removed
the
needs:design-review
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and the change also needs work. component:Resizable FR3 now says createResizable owns the behavior that ResizeHandle and useResizable go through; an engineering owner approves that clause, or asks for narrower wording, by commenting /approve-spec 57eccddb88ef560c25bd4b2d13d19d0414eb305c on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- The configuration-object
useResizableandcreateResizablekeep two copies of the region state; building that form on the factory would also make FR3's clause true. - The configuration-object hook now returns
isCollapsedandsizeone commit afteronCollapseChangefor every caller, SideNav included, while FR8 says they stay unchanged. useResizable(handleRef)on a plain element keeps the factory's listeners on a handle that has unmounted and announces "Resize handle" and "Collapsed" untranslated.- INV21 and INV22 deprecate the configuration-object form when the ref form arrives rather than in a later patch, and
LayoutPanelhands its pane to the region in a layout effect that itslendRefcallback ref can do. - FR8's browser verification covers
collapse()and Enter only, and at 6,454 authored lines the change needs splitting, with theresizableModel.tsextraction and the LayoutPanel slide as clean cuts.
[Automated review]
| } | ||
| regionPane.beforeCollapseChange(isCollapsed); | ||
| // eslint-disable-next-line @eslint-react/set-state-in-effect -- the new collapse state renders one synchronous commit after the measurement above, which needs the pane still at its old size; both commits land before paint | ||
| setShownIsCollapsed(isCollapsed); |
There was a problem hiding this comment.
This trailing commit applies to every configuration-object region, SideNav's included, whether or not a LayoutPanel lent a pane: a caller's effect first sees its onCollapseChange state collapsed while isCollapsed is still false and size still the old value, where FR8 says they stay unchanged. Measuring in collapse, expand, resize and _onResizeMove before they set state, as createResizable's commit() does, would leave the trailing commit only for a controlled isCollapsed change.
| if (regionPane == null) { | ||
| return; | ||
| } | ||
| regionPane.attachPane(paneLink.pane()); |
There was a problem hiding this comment.
This hand-off is work caused by the element attaching, and lendRef above already runs on attach and detach: calling regionPane?.attachPane(paneLink.pane()) there, with its cleanup and regionPane in its dependencies, removes this Effect (architecture:react-component-runtime INV2, INV3).
| | FR1 | ResizeHandle renders one focusable Handle and one enlarged Grab zone (a `--spacing-4` strip on the resize axis); the default render also contains a Grip pill, and caller `children` replace the pill. | Current source, docs, and tests | | ||
| | FR2 | The Handle carries `resize-handle`, the Grip pill carries `resize-handle-pill`, and the Grab zone carries no public target. | Current source and docs | | ||
| | FR3 | Pointer drag, keyboard, double-click collapse, snapping, persistence, reversal, and size behavior are owned by ResizeHandle and useResizable; dragging under right-to-left inverts the pointer delta so the divider follows the pointer. A disabled Handle ignores pointer and keyboard input and exposes `aria-disabled`, and keeps its tab stop (`spec:AST-085` FR1). With `autoSaveId`, storage holds the last settled size and receives the final size once when a drag ends, cancels, or unmounts. Implementation gap: a disabled Handle sets `tabIndex={-1}` (Current-state impact). | Current source, docs, and tests | | ||
| | FR3 | Pointer drag, keyboard, double-click collapse, snapping, persistence, reversal, and size behavior are owned by `createResizable`, which ResizeHandle and `useResizable` go through; dragging under right-to-left inverts the pointer delta so the divider follows the pointer. A disabled Handle ignores pointer and keyboard input and exposes `aria-disabled`, and keeps its tab stop (`spec:AST-085` FR1). With `autoSaveId`, storage holds the last settled size and receives the final size once when a drag ends, cancels, or unmounts. Implementation gap: a disabled Handle sets `tabIndex={-1}` (Current-state impact). | Current source, docs, and tests | |
There was a problem hiding this comment.
At this head only useResizable(handleRef, options) and a ResizeHandle given that ref go through createResizable; useResizable({...}) and a ResizeHandle with a resizable prop share only resizeGesture.ts and resizableModel.ts. Either build the configuration-object form on the factory or word the clause for the ref form; an engineering owner records the decision with /approve-spec <full head SHA>.
| const element = ref.current; | ||
| const entry = slot.get(); | ||
| if (entry != null && (element == null || entry.element === element)) { | ||
| return; |
There was a problem hiding this comment.
When a plain handle unmounts, this early return keeps the factory bound to the detached node: its 7 listeners and role/aria-label stay until the hook unmounts (react-component-runtime INV6). Destroying the entry and creating createResizable(null, …) seeded from readResizableSelection, as the hook does for a new element, handles the null detachment.
| import type {ResizableSize} from './utils'; | ||
|
|
||
| /** The English names the handle carries when no other is given. */ | ||
| const DEFAULT_LABEL = 'Resize handle'; |
There was a problem hiding this comment.
With useResizable(handleRef) on a plain element these English defaults reach aria-label and aria-valuetext in every locale (architecture:internationalization INV1a); the hook can pass @astryx.resizable.handle.label and @astryx.resizable.collapsed from the translator as the defaults, as ResizeHandle does.
57eccdd to
234bf19
Compare
vjeux
changed the title
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommended for approval: the helpers move into resizableModel.ts unchanged, useResizable keeps its behavior and warnings, and no public API changes. One optional wording fix: the changeset and the new module's header name createResizable and "the factory that follows", which this pull request does not add, so a release cut before the next stacked pull request lands would point readers at a missing factory. [Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] `useResizable`'s size parsing, bound resolution, clamping and snapping, saved-size storage and content-box measurement move unchanged into one internal module, so the factory that follows resolves, clamps, snaps and saves exactly as the hook does. No behavior changes. |
There was a problem hiding this comment.
Optional: this entry reaches the changelog under Other Changes, and createResizable is not part of this pull request. Something like "move unchanged into one internal module that a second caller can share. No behavior changes." stays accurate if the factory ships in a later release.
The first of four stacked pull requests for createResizable, split at its clean cuts: this extraction; then createResizable and the ref form, and the LayoutPanel slide, each opening as a draft stacked on the one before; then the deprecation of the configuration-object form (#9202), restacked last.
Maintainer impact
createResizable(the next pull request) has to resolve, clamp, snap and save sizes exactly asuseResizabledoes. Without one shared module, that logic would live twice, once in the hook and once in the factory, and the two could drift.Intended invariant
useResizable's behavior is unchanged: size parsing, bound resolution against a basis, clamping and snapping, saved-size storage and the content-box measurement give the same results, with the same warnings.Change
The pure parts of the resize model move, unchanged, from
useResizable.tsinto one internal module,resizableModel.ts, whichuseResizableimports. The module has no React and does no DOM work on import. It is not exported. It is maintenance, not product behavior: no output, prop, default or timing changes.A
[chore]changeset names@astryxdesign/core, which the changeset-coverage check requires for package source.Evidence
useResizable,ResizeHandle,LayoutandSideNavsuites pass unchanged.Scope
Testing
Local gates at 234bf19: unit tests (test:ui, 17,392 passed), check:repo, lint, root build then typecheck, the node project (only the known cache.test.mjs red), the Storybook build, accessibility in Chromium (77/77: ResizeHandle, Layout, the window splitter, SideNav and AppShell), and SideNav's rail-motion browser proof (10/10).