Repository navigation
feat(Resizable): useResizable(handleRef, options) is createResizable's thin React wrapper, and ResizeHandle renders the factory bound to its element - #9370
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
Requesting changes. The ref-taking useResizable(handleRef, options) follows the conformance path INV22 lists for useResizable, but this head leaves two copies of the resize logic: resizeGesture.ts repeats ResizeHandle's pointer and keyboard handling, and createResizable repeats the configuration form's size model, while both originals stay in use. Please move ResizeHandle's resizable path and the configuration form onto the factory in the change that introduces it, export CreateResizableOptions with the new overload, and land #9297 first so this diff (3,808 authored lines, 1,826 of them #9297's) drops under the 2,000-line scope limit or is split further.
[Automated review]
| model.cancel(); | ||
| }, | ||
|
|
||
| keyDown(event) { |
There was a problem hiding this comment.
This keyDown repeats ResizeHandle.tsx 683–781, and the pointer, snap-point, collapse and body-cursor helpers here repeat 619–680, 392–438 and 126–134. ResizeHandle still runs its own copy for the resizable path. Route that path through createResizeGesture in this change and delete the copy, so one input serves both.
There was a problem hiding this comment.
Moved into #9297: ResizeHandle's resizable path goes through createResizeGesture there, so this pull request adds no copy.
| } | ||
|
|
||
| /** The region's state from its options, its basis and its selection. */ | ||
| function compute(): void { |
There was a problem hiding this comment.
compute() repeats useSingleResizable's model in useResizable.ts (501–600, 811–824): the default that resolves once, the chosen-size latch, the bound re-clamp and the reported maximum. With both in use, a sizing fix reaches one form only. Build the configuration form on this factory in the same change, or extract the shared model both call.
There was a problem hiding this comment.
Moved into #9297: the configuration-object form builds on the factory there.
| hook usePowerSearchConfig: <const D extends ReadonlyArray<FieldDefinition>>(definitions: D, configName?: string) => { config: PowerSearchConfig; applyFilters: <T extends InferData<D>>(filters: ReadonlyArray<PowerSearchFilter>, data: ReadonlyArray<T>) => T[] } | ||
| hook usePressFeedback: () => typeof pressableProps | ||
| hook useResizable | ||
| overload (handleRef: RefObject<HTMLElement | null>, options?: CreateResizableOptions) => UseResizableResult |
There was a problem hiding this comment.
CreateResizableOptions appears in this public signature but no entry point exports it, so callers cannot name the new form's options. Export the type with this overload.
There was a problem hiding this comment.
Per the spec owners, CreateResizableOptions exports with createResizable and ResizableController in #9372, together with FR3.
| defaultIsCollapsed: previous.isCollapsed, | ||
| }; | ||
| const resizable = createResizable(element, createdWith); | ||
| readResizableInternals(resizable)?.setCollapsedText( |
There was a problem hiding this comment.
The translated collapsed text reaches the factory only through this internal channel, and the expanded size only through readResizableSelection, so a createResizable caller has neither. INV22 has the hook delegate entirely to a factory that takes the same options. Give the factory a collapsed-text option and an expanded-size reading before it is exported.
There was a problem hiding this comment.
Outside a provider the factory announces the shipped English catalog's text (architecture:internationalization INV3); how non-React callers localize it is pending a system-wide decision. The spec owners chose no public expanded-size member: a non-React caller's update() is synchronous, so its size is current.
There was a problem hiding this comment.
Update: collapsedLabel lands in #9372, defaulting to the catalog's "Collapsed" (architecture:internationalization INV3a, component:Resizable AR1).
5b99fc8 to
4905b79
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner deciding the option set of the new useResizable(handleRef, options) overload: its isReversed, isDisabled and label options beside ResizeHandle's own props, the unexported CreateResizableOptions type, and the attributes a plain element gets. An engineering owner can record that decision by commenting /approve-spec <full head SHA> (the author may, as one) or point to the governing record. Two changes are needed either way:
- The configuration form's
collapse(),expand()andresize()now do nothing when called before the hook's factory exists (for example from a child's mountuseLayoutEffect) or while an<Activity>hides the region, which worked on the base; please keep them working, add tests for those cases, and correct the "No behavior changes" changeset. - The diff is 5,090 authored lines, 3,836 of them from the still-open #9297; landing #9297 first and rebasing brings this pull request to about 1,410.
[Automated review]
| const getResizable = useCallback(() => slot.get()?.resizable ?? null, [slot]); | ||
| const collapse = useCallback(() => { | ||
| getResizable()?.collapse(); | ||
| }, [getResizable]); | ||
| const expand = useCallback(() => { | ||
| getResizable()?.expand(); | ||
| }, [getResizable]); | ||
| const resize = useCallback( | ||
| (next: number) => { | ||
| getResizable()?.resize(next); | ||
| }, | ||
| [getResizable], | ||
| ); |
There was a problem hiding this comment.
These controls reach only the factory the slot holds now. Before the first follow() layout effect, and after the hide cleanup sets the slot to null, getResizable() is null and the call is dropped, with no onCollapseChange or onSizeChange. On the base, resize(300) or collapse() from a child's mount useLayoutEffect, or collapse() while an <Activity> hides the region, takes effect. A no-factory path that applies the request to the slot's remembered selection, and fires the callbacks, would keep them working.
There was a problem hiding this comment.
Fixed in #9297: the factory lets go of the page while its component is unmounted or hidden and keeps the region, and a call before the first commit starts it early, so collapse(), expand() and resize() apply and report at once (tests there).
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] The configuration-object `useResizable` and `ResizeHandle`'s `resizable` path go through one internal resize factory and one input module, through the same size model as before, so the region's state and the handle's input each have one implementation. No behavior changes. |
There was a problem hiding this comment.
"No behavior changes" doesn't hold at this head: configuration-form controls called before the factory exists or while an <Activity> hides the region are now dropped, and its dev warnings start createResizable: instead of useResizable:.
There was a problem hiding this comment.
Both fixed in #9297: those calls apply (tests there), and the configuration-object form creates its factory as useResizable, so its warnings keep the hook's name and words (a test pins them against the base's).
4905b79 to
607c827
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner deciding the parts of the new useResizable(handleRef, options) that no current spec covers: the hook's isReversed and isDisabled options beside ResizeHandle's own props, the UseResizableResult type, the attributes a plain element gets, and the Grab zone change this branch carries. An engineering owner can record that decision by commenting /approve-spec <full head SHA> (the author may, as one), or point to the governing record. Two changes are needed either way:
- Export
CreateResizableOptionswith the overload and give it thecollapsedLabelthatcomponent:ResizableAR1 calls for, or merge this together with #9372. - Land #9297 and #9529 first and rebase: the diff is 5,945 authored lines, and 1,500 of them are this pull request's own.
[Automated review]
| const LABEL_KEY = '@astryx.resizable.handle.label'; | ||
| const COLLAPSED_LABEL_KEY = '@astryx.resizable.collapsed'; | ||
|
|
||
| export interface CreateResizableOptions { |
There was a problem hiding this comment.
useResizable(handleRef, options) makes this type public (api-report.md:2907), but no entry point exports it and it has no collapsedLabel, which component:Resizable AR1 gives it. Please export it with the overload and add collapsedLabel as the fallback for the collapsed text the hook passes in, or merge this together with #9372.
| /** | ||
| * @file createResizable.ts | ||
| * @input A handle element (or null, for a region with no handle yet) and the | ||
| * region's options: its size, bounds, snap | ||
| * points, collapse, persistence key, percentage basis, axis, and the | ||
| * handle's reversal, disabled state and name | ||
| * @output createResizable(handle, options) → {size, isCollapsed, collapse, | ||
| * expand, resize, update, subscribe, destroy}: a resizable region driven by | ||
| * a window-splitter handle, with no React (spec:AST-107 FR6; | ||
| * `architecture:public-component-api` INV22); readInitialResizableState, | ||
| * the state a region starts in, for the hook's first render; and, for the | ||
| * hook, createResizableAs, whose development warnings keep the hook's name | ||
| * and words, and the internal channel, which lets a factory go of the page | ||
| * and take it back while keeping its region (suspend, resume) | ||
| * @position The Resizable family's factory, internal to the package; its | ||
| * input is resizeGesture.ts. Both forms of useResizable are its React | ||
| * wrapper, and ResizeHandle drives the same input through it: over the | ||
| * region its `resizable` prop describes, or by rendering the factory bound | ||
| * to its element. Importing it does no DOM work, so it is safe on the server | ||
| * | ||
| * The factory never sizes the panel: it reports the size, through | ||
| * `onSizeChange` for an interaction and `subscribe` for every change, and the | ||
| * caller applies it, as a React caller renders it. | ||
| */ |
There was a problem hiding this comment.
Non-blocking: this header runs 24 lines, and its last paragraph repeats the JSDoc. Keep @input, @output and @position, and move "The factory never sizes the panel" into the createResizable JSDoc.
There was a problem hiding this comment.
Fixed in #9718, where the factory's element path now lives: the header is 11 lines, and the JSDoc says the factory never sizes the panel.
| // The factory follows the element behind the ref. A ref is stable across an | ||
| // element swap, so its current node is compared after every commit, and | ||
| // whenever a ResizeHandle given the ref attaches or lets go of its element, | ||
| // which a child can do without this component rendering. When the element | ||
| // goes away the factory on it goes too, with its listeners and attributes, | ||
| // and a factory with no handle keeps the region answering collapse, expand | ||
| // and resize; a new element gets a new factory, which starts where the old | ||
| // one's region stood. The same element takes the page back (`resume`). |
There was a problem hiding this comment.
Non-blocking: this comment covers several topics in sentences over 40 words. A shorter version that keeps the reason: "A ref keeps its identity when its element changes. Compare the element after each commit and when a ResizeHandle attaches or detaches. A new element gets a new factory that starts where the region stands."
There was a problem hiding this comment.
Fixed in aabb68f: the comment is in short sentences, one topic each.
607c827 to
aabb68f
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner deciding the parts of useResizable(handleRef, options) that no current spec covers: the hook's isReversed and isDisabled options beside ResizeHandle's own props (and how the two combine), the UseResizableResult type, the attributes a plain element gets, and the Grab zone change this branch carries, which narrows the --spacing-4 strip on short panes. An engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request (the author may, as one), or point to the governing record. Exporting CreateResizableOptions with collapsedLabel resolves the earlier options-type finding.
Two changes are needed either way:
- The diff is 6,978 authored lines, most of them from #9680, #9529, #9297 and #9718, so please land those first and rebase this onto them.
- The
isDisableddocs onCreateResizableOptionssay a plain element keeps its tab stop, but a disabled plain element getstabindex="-1", so please correct the docs or keep the tab stop on both paths.
[Automated review]
| * Ignores pointer and keyboard input and says so; a plain element keeps its | ||
| * tab stop. On a ResizeHandle, its own `isDisabled` prop does the same: |
There was a problem hiding this comment.
A disabled plain element does not keep its tab stop: announce() writes tabindex="-1" when the element has none of its own (separatorAttributes returns tabIndex: -1 while disabled), and createResizableHandle.test.ts asserts that. Please say it leaves the tab order, as ResizeHandle does, or keep the tab stop on both paths.
There was a problem hiding this comment.
Fixed in #9718, which this carries: a disabled plain element keeps its tab stop, as the docs say.
| name: 'handleRef', | ||
| type: 'RefObject<HTMLElement | null>', | ||
| description: | ||
| "The handle's ref, passed first: `useResizable(handleRef, options)` makes the element a window splitter and returns the region's size, collapse state and controls. Give the same ref to ResizeHandle, which reports its element as it mounts, or to a plain element, which the hook reads when its own component renders, so render a plain element there. The options are the params below, plus isReversed, isDisabled and label. Without a ref, the hook takes the params as one configuration object, for one region or several under `regions`, and also returns `props` for ResizeHandle.", |
There was a problem hiding this comment.
Non-blocking: this list of the ref form's extra options leaves out collapsedLabel, the option the changeset advertises (also in the dense description below).
There was a problem hiding this comment.
Taken in 4fe3151: the param text and the dense line list collapsedLabel.
aabb68f to
4fe3151
Compare
|
/approve-spec 4fe3151 |
…one separator derivation - createResizable on a plain element: the separator it makes, keeping a role, name and tab stop the element has, label and collapsedLabel, a pointer drag that holds its pointer (an iframe's element included), bounds and snap points, right-to-left and a reversed handle, the keys, Enter and a double-click, disabled input that leaves the tab order, saving through a drag, and a destroy that leaves nothing behind. - separatorAttributes: an open, collapsed, disabled and unbounded region, and no region. - ResizeHandle and a factory driving a plain element announce the same separator attributes, enabled, disabled and collapsed. - The region's tests and the configuration-object form's stand-in take the factory's handle argument, null for a region with no handle. They fail here: createResizable takes no handle, and separatorAttributes does not exist.
…esizeHandle and the factory derive the separator's attributes once createResizable(handle, options) takes a handle element, or null for a region with no handle yet. On an element it gives role="separator", a tab stop and a name when the element has none, its orientation and value, and the window splitter's input from resizeGesture.ts: a pointer drag that holds its pointer, the keys, Enter and a double-click. Its options add label, collapsedLabel, isReversed and isDisabled, and destroy takes all of it away. Its name and collapsed text are the shipped English messages, and collapsedLabel replaces the collapsed one. separatorAttributes derives the separator's ARIA and tab stop once. ResizeHandle renders it and the factory writes it on a plain element, so a disabled handle leaves the tab order in both. The configuration-object useResizable creates its factory with no handle. The factory stays internal, with a [chore] changeset.
…rrows to the labels key A factory test now covers a flat option: the factory announces collapsedLabel in place of the catalog's "Collapsed" on a plain handle. The gap that remains is a labels key.
…e and on a factory's plain element component:Resizable FR3 keeps a disabled Handle's tab stop, as spec:AST-085 FR1 does for every disabled control, so keyboard users can reach it and hear that it is disabled. ResizeHandle, the factory on a plain element and separatorAttributes each keep tabindex 0 while disabled. They fail here: a disabled separator leaves the tab order.
…sizable FR3 says separatorAttributes gave a disabled separator tabIndex -1, so a disabled ResizeHandle left the tab order and the factory wrote the same gap onto a plain element. FR3, after spec:AST-085 FR1, keeps a disabled Handle's tab stop so keyboard users can reach it and hear that it is disabled. The derivation now keeps tabIndex 0, and both still ignore input and say aria-disabled. A [fix] changeset records it.
…s evidence gap names the provider case component:Resizable no longer lists a disabled Handle leaving the tab order: its Current-state line goes, the disabled state row no longer calls the kept tab stop a gap, and the AST-085 relation no longer says FR3 records one. architecture:internationalization's INV3a evidence gap now reads: no factory test covers a labels key, or a flat option under a provider, yet.
…ew one and its target An accessibility [fix] names what changed from what, and toward what: before, a disabled ResizeHandle left the tab order; now it keeps its tab stop, as a disabled control does.
…d state's summary keeps it in the Tab order The fix below made FR3's implementation gap false: a disabled Handle keeps tabindex 0. FR3 drops that sentence, and its verification row no longer lists the disabled tab stop as untested, since ResizeHandle.test.tsx covers it. The disabled binding state's summary says the handle stays in the Tab order. A comment on the first-render read says why it starts as warned.
2e625c1 to
ccfe00a
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the parts of useResizable(handleRef, options) that no current spec covers. Two changes are needed either way.
An engineering owner needs to provide or identify the governing spec for:
- the hook and factory options
isReversedandisDisabled, whichcomponent:Resizablelists as Handle-only, and which OR with a boundResizeHandle's own props (the hook'slabelis ignored there); - the
UseResizableResultreturn type; - a plain element getting
role="separator", a tab stop and the catalog name, each only when it has none of its own.
An engineering owner can record the decision by commenting /approve-spec <full head SHA> on this pull request. The author may do so as one.
Blocking findings:
- The diff has 5,554 authored lines, and most of them come from the open drafts #9297 and #9718, so land those first and rebase so this pull request carries only the ref form's own changes (about 1,932 lines).
ResizeHandle.tsx:586usesview?.isReversed ?? isReversed, and before a factory binds the hook's starting view always setsisReversed, so a server-rendered<ResizeHandle ref={handleRef} isReversed />draws its pill on the wrong side until hydration; OR the two values, asisInputDisabledalready does, and add a server-render test.
Non-blocking: the record's "Builder impact" and compatibility rows still say "no API change".
[Automated review]
| const effectiveSide = resolveEffectiveSide( | ||
| pillPlacement, | ||
| isReversed, | ||
| view?.isReversed ?? isReversed, |
There was a problem hiding this comment.
Before a factory binds (server render and the hydrating render), view is the hook's starting view, and its isReversed is named.isReversed ?? false (useResizable.ts:714). That value is never nullish, so this handle's own isReversed is ignored. A server-rendered <ResizeHandle ref={handleRef} isReversed /> draws the pill with calc(-1 * …), as an unreversed handle does, then moves it after hydration. Suggest isReversed || !!view?.isReversed, matching isInputDisabled above, plus a renderToString test.
…e end A reversed handle controls the panel on its end side (ResizeHandle maps isReversed to an end panel), so the handle sits before that panel, not after it.
…en the ref, and its server markup - With a stand-in factory, useResizable(handleRef, options) creates it on the element, passes later options through update(), destroys it on unmount and returns what it reports. - A ResizeHandle given the ref renders and drives the region: keys, drag, a caller's onKeyDown first, collapse, its own isReversed and isDisabled, unmount mid-drag, a late handle, StrictMode, and both warnings, once each. A 0px region keeps the whole Grab zone, and a short pane limits it. - A handle a child mounts and unmounts from its own state, or hides in its own <Activity>, is bound at once. - Both forms report in the commit of onCollapseChange, the ref form keeps its size and collapse state across an <Activity>, and a handle that attaches before its container measures keeps the region's size. - A plain element behind the ref is named and announced with the provider's translation, unless the caller passes label. - On the server, the hook renders its initial size, and a ResizeHandle given the ref announces the region; a saved size hydrates with no mismatch. They fail here: useResizable takes no ref, and ResizeHandle binds to no factory.
…s thin React wrapper, and ResizeHandle renders the factory bound to its element useResizable(handleRef, options) creates the factory on the element behind the ref, follows the element when it changes, and passes the provider's translation of the collapsed text through the package's internal channel. A factory for a new handle takes the region's size as its selection, so an options update cannot replace it with the caller's default. A factory created on a ResizeHandle's element binds to the component through an internal registry instead of driving the element: ResizeHandle renders the separator's values from it and forwards its input to it, after a caller's onKeyDown. A handle a child mounts or hides on its own is bound at once, through ResizeHandle's callback ref. Until a factory binds, as on the server, ResizeHandle renders the view the hook recorded for its ref, so the server's markup announces the region and a hydrating render matches it. CreateResizableOptions is exported as a type, for the overload that takes it.
… its changeset and the collapsedLabel record lines useResizable's page documents useResizable(handleRef, options) beside the configuration-object form. The core API report gains the overload and the CreateResizableOptions rows, collapsedLabel included, and a [feat] changeset announces the form. component:Resizable loses the two lines that said collapsedLabel was not built yet: AR1's implementation-gap sentence and the Current-state line.
…f form's options The ref form's extra options are isReversed, isDisabled, label and collapsedLabel, the option its changeset names. The param text and the dense description now list all four.
…lets go, with no unmount Effect ResizeHandle interrupted a bound factory's gesture from an unmount-only Effect, while the claim callback ref already runs when the element lets go: at unmount and at an Activity hide. The interrupt moves there, before the claim is released, and the Effect and its gesture ref go.
… is new on every render A caller that passes an inline ref callback re-renders on each pointer move. React lets go of the merged ref and takes it back in that commit, and the handle's claim ref interrupted the drag then: the second move did not resize (240, not 280).
…really lets go React lets go of a handle's merged ref and takes the same element back in one commit whenever the caller's ref callback changes. The claim ref interrupted the gesture at the first half, so an inline ref callback stopped the drag after one move. The claim ref still releases and claims as before, and it interrupts the gesture after the commit only when its element did not come back.
…own isReversed Before a factory binds, ResizeHandle reads the hook's starting view, whose isReversed is never nullish, so a handle's own isReversed was ignored and its pill drew on the unreversed side until hydration.
… region's, as isDisabled does ResizeHandle placed its pill by `view?.isReversed ?? isReversed`, and the starting view's isReversed is always set, so a server-rendered handle ignored its own prop. Either value now reverses the handle.
…ct, and the follow comment keeps only its reason - Builder impact and compatibility now name the added useResizable(handleRef, options) form, which existing calls do not need. - The comment above the hook's follow keeps only why it compares the element after every commit.
ccfe00a to
c5dea36
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the parts of useResizable(handleRef, options) that no current spec covers, and one change is needed either way.
An engineering owner needs to provide or identify the governing spec for:
- the hook options
isReversedandisDisabled, whichcomponent:Resizablelists as Handle-only and which OR with a boundResizeHandle's own props (the hook'slabelis ignored there); - the
UseResizableResultreturn type; - a plain element getting
role="separator", a tab stop and the catalog name, each only when it has none of its own.
An engineering owner can record the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as one.
Blocking finding:
- The diff has 5,574 authored lines, 3,868 of them from the open drafts #9297 and #9718, so land those first and rebase so this pull request carries only the ref form's own changes (about 1,952 lines).
The server-markup reversal issue from the last review is fixed. Non-blocking: the record's Compatibility and migration bullet (Resizable.spec.md:75) still says "No prop, option, or return member changes".
[Automated review]
Stacked on #9718 and the PRs below it, whose commits this branch shows until they merge. Its own commits: vjeux/astryx@8652e74...c5dea36.
User need
spec:AST-107FR6 has each interaction hook be its factory's thin React wrapper. After #9718 the factory drives a plain handle element, but React callers have no form that takes the handle itself, and ResizeHandle cannot render a factory's region.Problem and solution fit
useResizable(handleRef, options)creates the factory on the element behind the ref, follows the element, and returnssize,isCollapsedand the controls in the commit ofonCollapseChange's state (component:ResizableFR8). A factory for a new handle takes the region's size as its selection.onKeyDown(FR7). Before a factory binds, as on the server, it renders the hook's recorded view.collapsedLabelreplaces it, on a plain element and on a ResizeHandle given the ref.Current specification
spec:AST-107FR6;architecture:public-component-apiINV22component:ResizableFR7, FR8 and AR1Public delta and caller burden
A
useResizable(handleRef, options)overload,UseResizableResult, and a type-onlyCreateResizableOptionsexport, in the API report. The configuration-object form and theresizableprop are unchanged.Design and alternatives
Evidence
The test commit fails before the change (32 failures). It covers the hook over a stand-in factory, ResizeHandle on a bound region and in server markup, a child-owned handle, commit timing, a late handle, and a plain element's translated name.
Scope
Writing rubric results
The change removes
component:Resizable's two lines sayingcollapsedLabeland the options form are not built. The record is owner-gated, so it waits for a spec owner's/approve-spec.Testing
Local checks at c5dea36, on main b377b21: core typecheck, the Resizable, Layout, SideNav and AppShell units (839 passed), strict eslint and prettier on the changed files, and the comment-length check. The test commits fail before their changes: 32 failures, then a drag through an inline ref callback that stops after one move (240, not 280), then a server-rendered handle that ignores its own isReversed. The full gates run at this head once CodeBunny approves.