Repository navigation
feat(Resizable): createResizable is public, and component:Resizable FR3 names it as the owner ResizeHandle and useResizable go through - #9372
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
AI review status for this pull request.
|
There was a problem hiding this comment.
createResizable, and the move of both useResizable forms and ResizeHandle onto it, fit the useResizable gap that architecture:public-component-api INV22 lists, but approval waits on an engineering-owner decision. An engineering owner needs to approve, for this exact head, the rewritten component:Resizable FR3 owner sentence and the public rows no current record names (ResizableController.update and subscribe, and the label, isReversed and isDisabled options) by commenting /approve-spec 062b726388ccc2efe77e3d943d83aef3eb846480; as an engineering owner, you can record it yourself. Two changes are still needed after that:
- A factory recreated before its container is measured (a ResizeHandle that mounts after the hook, or an
<Activity>hide and show of the configuration-object form) keeps and saves the temporary 1200px basis, so a'30%'default stays 360px instead of 240px (useResizable.ts:572-584). - The diff still carries #9297 and #9370 (5508 lines); merging those first leaves this pull request's own 1894 lines.
[Automated review]
| const previous = | ||
| entry == null | ||
| ? slot.remembered() | ||
| : readResizableSelection(entry.resizable); | ||
| entry?.resizable.destroy(); | ||
| const createdWith = | ||
| previous == null | ||
| ? optionsRef.current | ||
| : { | ||
| ...optionsRef.current, | ||
| defaultSize: previous.size, | ||
| defaultIsCollapsed: previous.isCollapsed, | ||
| }; |
There was a problem hiding this comment.
When the old factory was still on the temporary 1200px basis (a containerRef that is not laid out yet), this passes that placeholder size as a pixel defaultSize, so the new factory never resolves the percentage against the real container and saves the placeholder: defaultSize: '30%' stays 360px after the container measures 800px instead of 240px, with a ResizeHandle that mounts after the hook or an <Activity> hide and show. Carrying the previous selection only when it was made against a measured basis would keep AST-010's rule that the temporary basis is never final or persisted.
There was a problem hiding this comment.
Fixed in #9297: a factory made for a reveal or a late handle takes the region's size only when it was chosen against a measured basis, so a '30%' default resolves to 240px once the container measures. Tests cover both cases.
062b726 to
7a6ab57
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: component:Resizable FR3's reworded owner sentence and the public rows no current record names (ResizableController.update and subscribe, and the label, isReversed and isDisabled options) need a spec-owner approval for this exact head, which an engineering owner records by commenting /approve-spec 7a6ab579d185b006e69bbb75b1c656e387767184; you may record it yourself if you are an engineering owner. The earlier placeholder-basis issue is fixed, and ResizeHandle behaves as before in the stories checked, but three changes are still needed after that:
- A recreated factory drops a selection when its percentage bound's container measures 0: with
maxSize: '50%', a region resized to 300px comes back at its 200px default after an<Activity>hide and show (or a ref-form handle that attaches late), where the base keeps 300px (useResizable.ts:582-591). .changeset/create-resizable.mdsayslabelreplaces the announcement too; it names the handle only, and the collapsed state is always announced in English.- The diff still carries #9297 and #9370 (5654 lines); merging those first leaves this pull request's own 594 lines.
[Automated review]
| const createdWith = | ||
| previous == null | ||
| ? optionsRef.current | ||
| : { | ||
| ...optionsRef.current, | ||
| ...(previous.size === undefined | ||
| ? {} | ||
| : {defaultSize: previous.size}), | ||
| defaultIsCollapsed: previous.isCollapsed, | ||
| }; |
There was a problem hiding this comment.
The previous size reaches the new factory as a pixel defaultSize, and the options effect at lines 615-620 then calls update(options) with the caller's own defaultSize. When the new factory has a percentage bound and its container measures 0 at that moment, the default is not final yet, so the update replaces the carried size: with defaultSize: 200, maxSize: '50%', a region resized to 300px comes back at 200px once the container measures, after an <Activity> hide and show (the base keeps 300px) or a ref-form handle that attaches after resize(300). Passing the previous selection as the factory's selection (an internal seed for chosenSize) rather than as defaultSize would keep it through the update.
There was a problem hiding this comment.
Fixed in #9370 (the seed fix, unchanged by the split): a factory for a new handle takes the region's size as its selection through the internal channel, so the options update cannot replace it with the caller's defaultSize. A test attaches a handle before the container measures and checks the size.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `createResizable(handle, options)` makes a plain element a window splitter that resizes a region, without React: a pointer drag that holds its pointer across embedded frames, the arrow keys, Home, End, Enter and a double-click, with bounds, snap points, collapse and saved sizes. It reports the size through `subscribe` and `onSizeChange`, and you size the panel; `destroy` removes every listener and attribute it added. The handle is named and announced with the message catalog's English messages, unless you pass `label`. `useResizable(handleRef, options)` is its React form. |
There was a problem hiding this comment.
label sets only the handle's name; a bare factory always announces its collapsed state with the English catalog's "Collapsed" (createResizable.ts:805-808). Suggested: "The handle is named with the catalog's English message unless you pass label, and announces its collapsed state in English."
There was a problem hiding this comment.
Taken: the changeset says the handle is named and announced with the catalog's English messages, label names it, and collapsedLabel replaces "Collapsed".
7a6ab57 to
a2565f2
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: component:Resizable FR3's reworded owner sentence, the public rows no current record names (ResizableController.update and subscribe, and the isReversed and isDisabled options), and the Grab zone change this branch carries from #9529 need a spec-owner approval for this exact head, which an engineering owner records by commenting /approve-spec a2565f2b371a47dd133fefe5aaf5a8dbb758ae74; you may record it yourself if you are an engineering owner. The earlier <Activity> selection loss and the changeset wording are fixed, but three changes are still needed after that:
- A non-collapsible pane at 0px now gets a 0px Grab zone, so a drag from the divider can no longer open it (
ResizeHandle.tsx:539). useResizable(handleRef, options)with inline options drops aresize(300)back to its 200px default when the handle attaches while a percentage-bound container measures 0 (useResizable.ts:576-585).- The diff still carries #9297, #9529 and #9370 (6638 lines); merging those first leaves this pull request's own 731 lines.
[Automated review]
| // The pane the Handle resizes, while it has any length: an inline zone | ||
| // reaches at most half of it, and an overlay Handle takes at most half of | ||
| // it. A collapsed pane has nothing to protect and keeps the whole zone. | ||
| const paneLength = isCollapsed ? undefined : (resizable?._size ?? view?.size); |
There was a problem hiding this comment.
A pane at 0px that is not collapsed (for example minSize: 0 without collapsible) passes 0 here, so the Grab zone becomes min(var(--spacing-4), 0px) wide (and an overlay Handle min(16px, 50%) of nothing). The zone is the only element with pointer handlers, so a drag from the divider no longer opens the pane; the base opens it. Treating a 0px pane like a collapsed one here, or giving the zone a floor, keeps it reachable.
There was a problem hiding this comment.
Fixed in #9529, below this: a 0px pane that is not collapsed keeps the whole Grab zone.
| const createdWith = | ||
| previous == null | ||
| ? optionsRef.current | ||
| : { | ||
| ...optionsRef.current, | ||
| ...(previous.size === undefined | ||
| ? {} | ||
| : {defaultSize: previous.size}), | ||
| defaultIsCollapsed: previous.isCollapsed, | ||
| }; |
There was a problem hiding this comment.
The carried selection reaches the new factory only as defaultSize. With inline options, the effect at lines 618-623 then calls update(options) with the caller's own defaultSize, and while a percentage bound's container measures 0 the default is not final, so it replaces the carried size: defaultSize: 200, maxSize: '50%', resize(300), then a handle attaches, and the region comes back at 200px once the container measures. Seeding the new factory's selection through the internal channel would keep it through the update.
There was a problem hiding this comment.
Fixed in #9370, as in the thread above: the new factory's selection is seeded through the internal channel, so an inline defaultSize in the next update() cannot replace the carried size.
| * @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) |
There was a problem hiding this comment.
Non-blocking: this @output lists internal exports and their purposes in one long sentence ("lets a factory go of the page" reads unclearly). Something like "@output createResizable, and the internal reader and channel useResizable uses" keeps the header short.
There was a problem hiding this comment.
Fixed: @output reads "createResizable; readInitialResizableState, createResizableAs and the internal channel for the hooks".
a2565f2 to
5c82b3e
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision, and three changes are needed besides.
An engineering owner needs to approve, for this exact head, the reworded component:Resizable FR3 owner sentence ("owned by createResizable, which ResizeHandle and useResizable go through"), the public rows no current record names (ResizableController.update and subscribe, and the isReversed and isDisabled options), and the ResizeHandle Grab zone change this branch carries (centred on the divider, a pointer-taking pill, and a cut on panes shorter than 16px). An engineering owner records it by commenting /approve-spec 5c82b3e03f2eecb426a2956c8aefb1e3c527dc8c; you may record it yourself if you are an engineering owner.
Required changes:
- A disabled plain element driven by
createResizableleaves the tab order (createResizable.ts:853-855, asserted bycreateResizableHandle.test.ts:276-283), while the option's JSDoc and the doc page say it keeps its tab stop andspec:AST-085FR1 requires that; keep the tab stop, or correct the docs and record the gap in FR3. - FR1 still states a fixed
--spacing-4Grab zone and the ownership boundary an aligned one, while the branch centres it and cuts it on short panes; amend both in this pull request or drop that change. - The diff is 7519 authored lines, of which this pull request's own commits are 575; land #9680, #9529, #9297, #9718 and #9370 first and rebase.
The earlier zero-height Grab zone and the ref-form selection loss are fixed, and the Resizable, Layout, SideNav and AppShell suites, the core typecheck and lint pass locally.
[Automated review]
| if (original.get('tabindex') == null) { | ||
| setAttribute('tabindex', String(aria.tabIndex)); | ||
| } |
There was a problem hiding this comment.
A disabled plain element gets tabindex="-1" here, since separatorAttributes() returns -1 when disabled, so it leaves the tab order the factory gave it. spec:AST-085 FR1 keeps a disabled control's focus order, and this option's own JSDoc and doc page say "a plain element keeps its tab stop". Keeping 0 here (or in separatorAttributes for both paths, which also closes ResizeHandle's recorded FR3 gap) would match them; otherwise the docs need to say it leaves the tab order.
There was a problem hiding this comment.
Fixed in #9718, which this carries: separatorAttributes keeps tabIndex: 0 while disabled, on both paths, which closes ResizeHandle's recorded FR3 gap too.
| name: 'options.isDisabled', | ||
| type: 'boolean', | ||
| description: | ||
| 'Ignores the drag and the keys and sets `aria-disabled`. A plain element keeps its tab stop, so it stays discoverable. On a ResizeHandle, its own `isDisabled` prop does the same: either one disables the handle.', |
There was a problem hiding this comment.
This promise does not hold at this head: createResizableHandle.test.ts:276-283 asserts a disabled plain element's tabIndex is -1. Either keep the tab stop in the factory or say here that a disabled handle leaves the tab order, as ResizeHandle's does today.
There was a problem hiding this comment.
It holds now: with #9718's fix, a disabled plain element keeps its tab stop, and the test asserts 0.
| @@ -147,7 +143,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | --- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------- | | |||
| | 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 | | |||
There was a problem hiding this comment.
The branch now centres the Grab zone whatever pillPlacement says and cuts it to min(--spacing-4, pane) on a short pane (ResizeHandle.tsx:553-555, :729-733), and a visible pill takes the pointer, but FR1 (and the ownership line's "aligned invisible Grab zone") still describe a fixed strip. Amending them here, or dropping the change from this branch, keeps the record and the code in step.
5c82b3e to
1836bc3
Compare
…s the factory again after another one A caller that switches between two stable configuration objects, WIDE, then NARROW, then WIDE, must get WIDE's bounds back. It fails here: the hook compares each configuration with the one it created the factory with, so the last switch never reaches update().
…e factory has now The hook kept the options it created the factory with and compared every later configuration against them. So WIDE, then NARROW, then WIDE skipped the last update() and kept NARROW's bounds. The slot entry now holds the options last passed to the factory, at creation or at an update.
…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.
…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.
… drag-across-a-frame proof and API report rows (spec:AST-107 FR6) createResizable, CreateResizableOptions and ResizableController are exported from the Resizable entry point and the root, in the pull request whose FR3 line names createResizable as the owner ResizeHandle and useResizable go through, so the public API and its record line land together. createResizable is documented as a hook beside useResizable (no category, so it is not a tile in the components gallery; the docsite's route lock names its page, and the doc compiler's pin reads it by name); its page says update() takes the full set of options, and useResizable's page names it as the factory both forms wrap. The story drives a plain divider over an embedded frame, and the browser proof drags across that frame, which the held pointer keeps from swallowing the drag. The core API report gains the export rows (only those), and the changeset announces the factory.
…and useResizable go through component:Resizable FR3 owned the drag, keys, double-click, snapping, persistence, reversal and size in ResizeHandle and useResizable. Both forms of the hook and both paths of the handle go through createResizable, now public, so FR3 names it as the owner they go through (architecture:public-component-api INV22).
…abel The doc page lists options.collapsedLabel, the text a collapsed handle announces in place of the catalog's "Collapsed", and its guidance says the collapsed text outside a provider is the shipped English one unless collapsedLabel is passed. The changeset says label names the handle and collapsedLabel replaces its collapsed text.
…, with no record citations Each comment sentence the public export adds has at most 20 words, the label option's JSDoc cites no record, and the browser proof's header fits in about 12 lines. No code change.
…one is centred on the Handle The ownership line called the Grab zone "aligned", which reads as aligned with the pill. FR1 now centres the zone on the Handle for every pillPlacement, so the line says that.
… internal ones Two changesets called the factory internal while this one announces it as public in the same release. They fold into it: the configuration-object useResizable and ResizeHandle build on the factory, with no behavior change.
…are listed separately
988c942 to
ab4e70f
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision, and one change is needed besides.
An engineering owner needs to approve, for this exact head, what no current spec states yet:
- the reworded
component:ResizableFR3 owner sentence ("owned bycreateResizable, which ResizeHandle anduseResizablego through"); - the public
ResizableController.update()andsubscribe(), and theisReversedandisDisabledoptions ofCreateResizableOptionsanduseResizable(handleRef, options), which combine with a ResizeHandle's own props; - the Grab zone change this branch carries from #9529: FR1's three added sentences (now including that a hover-revealed pill takes the pointer across the gap toward the Grab zone, never past its far side) and the ownership line "invisible Grab zone centred on the Handle", including whether
design:bidirectionalityDR8 and DR12 need an amendment, since an off-centre pill no longer shares a centre with the zone.
An engineering owner records that decision by commenting /approve-spec ab4e70f2b275608c501dcb36eb45342eb9ea7199 on this pull request; the author may do so as an engineering owner.
Required change:
- The diff is 6955 authored lines, of which this pull request's own commits are 593; land #9529, #9297, #9718 and #9370 first and rebase so this pull request carries only its own change.
Non-blocking: architecture:public-component-api's INV22 known-gap list still names useResizable, and the HoverPillOnOverlayHandle story renders its overlay handle at the frame's edge rather than at the sidebar (inline).
[Automated review]
| <LayoutPanel width={sidebar.size} hasDivider={false}> | ||
| <Text>{sidebar.size}px</Text> | ||
| </LayoutPanel> | ||
| <ResizeHandle |
There was a problem hiding this comment.
Non-blocking: this overlay ResizeHandle sits beside the LayoutPanel with no positioned ancestor, so it renders at the frame's edge (x 1264-1280 in a 1280px frame), not at the 250px sidebar it resizes. The pill-gap checks still hold, since they are geometric, but the fixture is not a real overlay layout. Wrapping it in the panel or a position: relative box, as SideNav does, would fix it.
Stacked on #9370 and the PRs below it, whose commits this branch shows until they merge. Its own commits: vjeux/astryx@2e625c1...ab4e70f.
User need
architecture:public-component-apiINV22 has every public interaction hook delegate to a publiccreate…(element, options)factory (spec:AST-107FR6). After #9370 both forms of useResizable and ResizeHandle go through createResizable, but the factory is internal. A page without React cannot make a divider a window splitter, andcomponent:ResizableFR3 still names ResizeHandle and useResizable as the owners of the behavior.Problem and solution fit
createResizable(handle, options)and itsResizableControllertype are exported from@astryxdesign/core/Resizableand the root. It makes a plain element a window splitter without React; the caller sizes the panel.component:ResizableFR3 names createResizable as the owner that ResizeHandle and useResizable go through.update()takes the full set of options. useResizable's page names it as the factory both forms wrap, and a changeset announces the factory.Current specification
spec:AST-107FR6;architecture:public-component-apiINV22component:ResizableFR3 (edited here)Public delta and caller burden
New exports:
createResizableandResizableController, with their API report rows. Nothing existing changes.Design and alternatives
update()is synchronous, so itssizeis already current.Writing rubric results
Over
component:Resizable: FR3's owner sentence changes, and the ownership line now centres the Grab zone on the Handle. The record is owner-gated, so it waits for a spec owner's/approve-spec.Evidence
Scope
Testing
Local checks at ab4e70f: core typecheck, the Resizable, Layout, SideNav and AppShell units (837 passed), strict eslint and prettier on the changed files, and the comment-length check. The full gates run at this head once CodeBunny approves.