Repository navigation
feat(Resizable): createResizable drives a plain handle element, with one separator derivation - #9718
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| SideNav | 70.9 kB (+690 B) · 11.2 kB CSS (0) | — | — · — · — |
| ResizeHandle | 19.5 kB (+47 B) · 4.3 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.26 MB packed (+4.9 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 1 row; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
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.
This review needs an engineering owner's decision, and it also asks for changes that stand whatever that decision is.
Owner decision needed. This branch carries the Grab zone changes from #9529. The zone stays centred on the divider whatever pillPlacement says, it narrows to half of a short pane, and the Grip pill now starts a drag. The Resizable component record states only a --spacing-4 Grab zone strip. An engineering owner (the author qualifies) needs to name the current clause that governs these behaviors, or approve them as an amendment to that record.
Required changes:
- Land #9680, #9529 and #9297 first and rebase: today the diff carries 5,269 authored lines from four pull requests, and only 1,018 of them are this pull request's own commits.
- Keep a disabled factory handle's tab stop:
separatorAttributesreturnstabIndex: -1while disabled, but the Resizable record (FR3) and the factory's ownisDisableddoc say a disabled handle keeps its tab stop. - Keep the provider case in INV3a's evidence gap: the new factory tests cover a flat option only outside a provider, so the gap should read "no factory test covers a
labelskey, or a flat option under a provider, yet".
The factory path, the shared separator derivation and the carried hydration and mid-drag fixes look right. The Resizable, Layout, SideNav and AppShell suites and the core typecheck pass at this head.
[Automated review]
| : undefined, | ||
| 'aria-valuetext': value?.isCollapsed ? value.collapsedText : undefined, | ||
| 'aria-disabled': state.isDisabled || undefined, | ||
| tabIndex: state.isDisabled ? -1 : 0, |
There was a problem hiding this comment.
A disabled separator leaves the tab order here, and the factory writes this on a plain element. The Resizable component record's FR3 says a disabled Handle keeps its tab stop, and so does the factory's own isDisabled doc. Return 0 here (and update FR3's ResizeHandle gap note and the parity tests), or keep the tab stop in the factory.
There was a problem hiding this comment.
Fixed in 5f4a410: separatorAttributes keeps tabIndex: 0 while disabled, so a disabled ResizeHandle and a factory's plain element both keep their tab stop, as FR3 says. The record drops its gap lines, and a [fix] changeset records it. The tests fail before the fix.
| | INV2, INV13, INV14 | export drift checks, public i18n tests, compatibility review | A parallel/replacement runtime lands without migration, or client hooks claim server behavior | | ||
| | INV3–INV5 | resolver and provider rerender tests | no-provider output depends on the host, locale swaps stay stale, fallback order changes, or malformed-locale behavior is described as graceful | | ||
| | INV3a | factory unit tests: a flat option or `labels` key replaces its message, including under a provider, and outside a provider its absence announces or shows shipped English; wrapper tests: under a non-English provider its absence gives the translation. Evidence gap: no factory test covers a flat option or `labels` key yet | a factory announces or shows Astryx text that no option or `labels` key can replace, groups more than one string outside one `labels` object, takes a translator, ignores a given string, or gives something other than the catalog message when it is absent | | ||
| | INV3a | factory unit tests: a flat option or `labels` key replaces its message, including under a provider, and outside a provider its absence announces or shows shipped English; wrapper tests: under a non-English provider its absence gives the translation. Evidence gap: no factory test covers a `labels` key yet | a factory announces or shows Astryx text that no option or `labels` key can replace, groups more than one string outside one `labels` object, takes a translator, ignores a given string, or gives something other than the catalog message when it is absent | |
There was a problem hiding this comment.
The new factory tests cover a flat option only outside a provider, and this row asks for the case under a provider too. Suggest: "Evidence gap: no factory test covers a labels key, or a flat option under a provider, yet."
There was a problem hiding this comment.
Taken in 5f4a410: "no factory test covers a labels key, or a flat option under a provider, yet".
d76b9f9 to
5f4a410
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's spec decision, and it also needs changes that stand whatever that decision is.
Owner decision needed. The branch carries the Grab zone changes from #9529 and writes them into the Resizable record's FR1: the zone stays centred on the divider whatever pillPlacement says, shrinks so a short pane keeps half of itself, and the Grip pill takes the pointer. The current FR1 states only a --spacing-4 strip. The same FR1 text was approved on #9529's head, but that approval does not carry over to this head. An engineering owner records the decision by commenting /approve-spec with this pull request's full head SHA (the author may, as an engineering owner), or #9529 lands first. The decision should also square the shrunk zone and an offset pill with the bidirectionality record's DR8, which keeps a pointer target centred on the visible control at its required size.
Required changes:
- Land #9680, #9529 and #9297 first and rebase: the diff carries 5,290 authored lines from four pull requests, and only 1,037 of them are this pull request's own commits.
- Delete FR3's "Implementation gap: a disabled Handle sets
tabIndex={-1}" sentence and the matching verification-row gap in the Resizable record, which this pull request's own fix makes false, and reword the disabled state's summary inResizeHandle.a11y.states.ts.
The factory now keeps a disabled handle's tab stop and INV3a's evidence gap names the provider case, as asked last time. The Resizable, Layout and SideNav suites and the core typecheck pass at this head.
[Automated review]
| | 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 | | ||
| | 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. The grab zone is centred on the Handle for every `pillPlacement`, and the painted pill is its own grab target. On a pane too short for the full strip, the zone shrinks so the pane keeps at least half of itself. | Current source, docs, tests, and an owner ruling | | ||
| | 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 | |
There was a problem hiding this comment.
This FR3 sentence still says a disabled Handle sets tabIndex={-1} and points at a Current-state entry this pull request deletes, while the code now keeps tabIndex 0. Drop "Implementation gap: a disabled Handle sets tabIndex={-1} (Current-state impact)."
| | FR1 | `ResizeHandle.test.tsx`; stable visual baseline `core-resizable--horizontal-split` | Default, collapsed, both axes | Handle and Grab zone regressions fail focused tests; a changed default handle comes back changed for review. Gap: no test renders `children` to prove they replace the Grip pill. | `audit:Resizable/anatomy` | | ||
| | FR1 | `ResizeHandle.test.tsx`; `grabZone.interaction.browser.spec.ts` for the zone's place and its short-pane limit; stable visual baseline `core-resizable--horizontal-split` | Default, collapsed, both axes | Handle and Grab zone regressions fail focused tests; a changed default handle comes back changed for review. Gap: no test renders `children` to prove they replace the Grip pill. | `audit:Resizable/anatomy` | | ||
| | FR2 | `themingTargets.test.ts`; source review | Handle, Grip pill, Grab zone | A removed target fails the inventory. `themingTargets.test.ts` catches only a renamed or undocumented target; no check fails if the Handle or Grip pill stops carrying its target. | `audit:Resizable/theming` | | ||
| | FR3 | `ResizeHandle.test.tsx` and `useResizable.test.ts`; storage timing: `ResizeHandle.test.tsx` ("saves the size once, when a drag ends on …", "saves the dragged size once when the split unmounts mid-drag", "still saves each keyboard step at once") and `useResizable.test.ts` ("writes the size a drag reached once, after … releases it", "writes the final size once when the release comes before the last move renders", "keeps the size a region was dragged to when it unmounts mid-drag", "writes only the re-clamped size when the container shrank mid-drag") | Pointer, keyboard, snap, collapse, persistence; RTL drag; disabled keyboard | A changed transformation, an RTL drag that moves against the pointer, keyboard input that resizes a disabled Handle, or a per-move, stale or missing final storage write fails. Gap: no test covers disabled pointer input or the disabled tab stop. | `audit:Resizable/behavior` | |
There was a problem hiding this comment.
ResizeHandle.test.tsx ("ignores keyboard input when disabled, and keeps its tab stop") now covers the disabled tab stop. End this gap at "no test covers disabled pointer input".
There was a problem hiding this comment.
Narrowed in 1aca7db: the gap now ends at disabled pointer input.
| return direction === 'vertical' ? window.innerHeight : window.innerWidth; | ||
| } | ||
|
|
||
| /** A warning already given, so a resolution repeated silently stays quiet. */ |
There was a problem hiding this comment.
Non-blocking (INV29): this comment is vague. Suggest: "Starts as warned. The factory warns; this first-render read does not."
There was a problem hiding this comment.
Taken in 1aca7db, slightly reworded: "Starts as warned: the factory warns, and this first-render read does not."
5f4a410 to
1aca7db
Compare
|
/approve-spec 1aca7db |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: two changes stand, and the spec question is settled.
- Land #9529 and #9297 first and rebase: the diff carries 4,602 authored lines, and only 1,045 of them are this pull request's own commits.
- Add a one-line note to the new hover-grace Effect in
ResizeHandle.tsxnaming what it synchronizes (the pill's hover timer, cleared on unmount), as the React component runtime record's INV3 requires of every new Effect.
The Resizable record's FR1 Grab zone amendment, with the 150 ms pill hover grace, is approved at this head, and FR3's stale tab-stop gap is fixed. The Resizable, Layout, SideNav and AppShell suites and the core typecheck pass at this head.
[Automated review]
| const handleRef = useRef<HTMLDivElement>(null); | ||
| const [isHovered, setIsHovered] = useState(false); | ||
| const hoverExitRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
| useEffect( |
There was a problem hiding this comment.
This new Effect names no external system, where architecture:react-component-runtime INV3 asks every new Effect to. It is needed (nothing else runs at unmount to clear the 150 ms timer onPointerLeave starts), so one line above it is enough, for example:
// The pill's hover grace timer: cleared on unmount, so it sets no state after.| } | ||
| } | ||
|
|
||
| // --- The handle's attributes --- |
There was a problem hiding this comment.
Non-blocking (INV29): this divider, and the ones at lines 766 and 910, restate the code under them. Drop them.
1aca7db to
6a1ed94
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's decision, and changes that stand either way.
Owner decision needed. A hover-revealed Grip pill now gets an invisible 13px extension in ResizeHandle.tsx that keeps the hover and starts a drag, in place of the approved 150 ms hover grace, while the Resizable record's FR1 names only the Grab zone and the painted pill as grab targets. An engineering owner either names the current clause that covers it, or approves it as part of FR1 by commenting /approve-spec <full head SHA> once FR1 states it (the author may do so as an engineering owner).
Required changes:
- Land #9529 and #9297 first and rebase: the diff carries 4,626 authored lines, and only 1,043 of them are this pull request's own commits.
- Add a one-line note to the options Effect in
useResizable.tsnaming what it synchronizes (the factory it passes new options to), as the React component runtime record's INV3 requires of every changed Effect.
The plain-handle path, the shared separator derivation and the disabled tab-stop fix look right, and the Resizable, Layout, SideNav and AppShell suites and the core typecheck pass at this head.
[Automated review]
| }, | ||
| [slot], | ||
| ); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This changed Effect names no external system, where architecture:react-component-runtime INV3 asks every new or changed Effect to. It is needed (update reaches the factory, a store outside React, and cannot run during render), so one line above it is enough, for example:
// The factory: each new options object reaches it through update(), as a new render's would.| pointerEvents: 'auto', | ||
| backgroundColor: colorVars['--color-border-emphasized'], | ||
| }, | ||
| // A pill shown on hover can sit outside the Grab zone. This invisible |
There was a problem hiding this comment.
Owner decision needed: this invisible extension keeps a hover-revealed pill shown and starts a drag in the gap beside the Grab zone, while the Resizable record's FR1 names only the zone and the painted pill as grab targets. An engineering owner either names the clause that covers it or approves it as part of FR1.
6a1ed94 to
c6f6d33
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and one change stands whatever that decision is. The Resizable record's FR1 at this head adds the Grab zone rules carried from #9529 (centred on the divider for every pillPlacement, at most half of a short pane, the painted pill as its own grab target, and a hover-revealed pill that takes the pointer across the gap to the zone but never past its far side); the same text is approved on #9529's head, but no approval covers this pull request's head. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner.
- Land #9529 and #9297 first and rebase: the diff carries 4,681 authored lines, and only 1,042 of them are this pull request's own commits.
The disabled-handle tab stop fix checks out, and two optional wording notes remain: the changeset note inline, and the record's AR1 and Current-state entries, which still call createResizable unbuilt.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] The internal resize factory can drive a plain handle element: it gives the element the separator's role, name, value and tab stop, and answers its pointer drag, keys, Enter and double-click. ResizeHandle and the factory derive the separator's attributes from one helper, with no behavior change. |
There was a problem hiding this comment.
Non-blocking: the shared helper is also what moves a disabled ResizeHandle's tabindex from −1 to 0, which the [fix] changeset records. Maybe: "…derive the separator's attributes from one helper, with no other behavior change."
There was a problem hiding this comment.
Agreed. #9372 folds this changeset into the public createResizable one, so the wording lands there, in its next round.
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested for scope only: the factory's plain-handle path, the shared separator derivation and the disabled tab-stop fix match the current records.
- Land #9297 first and rebase: the diff carries 3,772 authored lines, and 2,808 of them are #9297's commits, which is still open on its own.
Two optional wording notes: the Resizable record's Current-state entry and AR1 gap still say createResizable is not built (it now is, internal only), and the plain-handle changeset's "with no behavior change" sits beside the tab-stop [fix]. The record edits only drop gap notes this pull request closes, though the repository's spec gate still waits for an engineering owner's /approve-spec at this head. The Resizable, Layout, SideNav and AppShell suites, the core typecheck and strict lint pass at this head.
[Automated review]
There was a problem hiding this comment.
Changes requested for scope only: the factory's plain-handle path, the shared separator derivation and the disabled tab-stop fix match the current Resizable record.
- Land #9297 first and rebase: the diff carries 3,772 authored lines, and 2,808 of them are #9297's commits, which is still open on its own.
Two small follow-ups, not blocking: the Resizable record still says createResizable is not built (its Current-state impact and AR1's implementation gap), and the plain-element test named "keeps a role, a name and a tab stop" never checks a role. Because the pull request edits current records, the repository's spec gate also waits for an engineering owner's /approve-spec with the full head SHA.
[Automated review]
| expect(handle.tabIndex).toBe(0); | ||
| }); | ||
|
|
||
| it('keeps a role, a name and a tab stop the element already has', () => { |
There was a problem hiding this comment.
Non-blocking: this test is named for a role the element already has, but it never sets or checks one, so the role branch in createResizable.ts (original.get('role') == null) has no test. Suggest giving the element a role of its own before create() and asserting it stays.
…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.
…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.
8652e74 to
3705e58
Compare
There was a problem hiding this comment.
Recommending approval: the factory's plain-handle path, the shared separator derivation and the disabled tab-stop fix match the current Resizable records, and no blocking issue remains now that the diff holds only this pull request's own commits.
Non-blocking, here or in #9372:
createResizableHandle.test.ts:117is named for a role the element already has but never sets one, so the role guard atcreateResizable.ts:734is untested and the JSDoc at line 339 says the role is always set; add a role to the element, assert it stays, and say so in the JSDoc.- The
[chore]changeset says the shared helper brings "no behavior change", while it is what moves a disabledResizeHandle'stabindexfrom -1 to 0. Resizable.spec.mdstill sayscreateResizableis not built (the Current-state entry and AR1's gap); say it is internal and not exported.- Two comments (
ResizeHandle.tsx:542-543,createResizable.ts:741) restate what theseparatorAttributescall already says; delete them.
An engineering owner has already approved the record edits at this head.
[Automated review]
| * Make `handle` a window splitter that resizes a region. The factory never | ||
| * sizes the panel: it reports the size, and the caller applies it. | ||
| * | ||
| * It gives the handle `role="separator"`, its orientation and its value. It |
There was a problem hiding this comment.
Non-blocking: the factory keeps an element's own role (line 734), so this sentence overstates it. Suggest: "It gives the handle role=\"separator\" when it has no role, and its orientation and its value."
| // The attributes createResizable writes on a plain element, from the same | ||
| // derivation. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates what the separatorAttributes call and that file's header already say. Delete it.
| setAttribute('aria-label', opts.label ?? english[LABEL_KEY]); | ||
| } | ||
| const {size, isCollapsed, min, effectiveMax} = derived; | ||
| // The attributes ResizeHandle renders, from the same derivation. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates what the separatorAttributes call and that file's header already say. Delete it.
|
/approve-spec 3705e58 |
github-actions
Bot
removed
the
needs:spec-owner-review
cixzhang
added
the
ci:run
Rebased onto main dcf3d6f, which holds #9297's merge. #9370's ref form calls this path.
Maintainer impact
spec:AST-107FR6 has each interaction hook be its factory's thin React wrapper, so the factory must drive a handle element. This gives the internal factory that path and one separator derivation shared with ResizeHandle, split from #9370.Intended invariant
No public API or visual change; the factory stays internal until #9372 exports it. One behavior changes, a
[fix]that restorescomponent:ResizableFR3 (spec:AST-085FR1): a disabled ResizeHandle keeps its tab stop, so keyboard users can reach it and hear that it is disabled.Change
createResizable(handle, options)takes an element, or null for a region with no handle. On an element it adds the separator's role, tab stop, name, orientation and value, and the input ofresizeGesture.ts;destroyremoves them. The options addlabel,collapsedLabel,isReversedandisDisabled, with shipped English as the default text.separatorAttributes.tsderives the separator's ARIA and tab stop. ResizeHandle renders it and the factory writes it, so a disabled handle keeps its tab stop in both.[chore]and[fix]changesets. INV3a's evidence gap narrows to alabelskey or a flat option under a provider, and component:Resizable drops the closed tab-stop gap.Evidence
separatorAttributes; 3 failures: a disabled handle left the tab order).Scope
[fix]above.Writing rubric results
Over
architecture:internationalization(INV3a's evidence gap) andcomponent:Resizable(the closed tab-stop gap, in FR3, its verification row, the Current-state line, the state row and the relation). The records are owner-gated, so they wait for a spec owner's/approve-spec.Testing
Local checks at 3705e58, on main dcf3d6f: a frozen install, core typecheck, the Resizable, Layout, SideNav and AppShell units (801 passed), strict eslint and prettier on the changed files, check:knowledge, check:changesets, the comment-length check, and
pnpm review:pre(1,042 lines, no findings). The test commits fail before their changes (36 failures, then 3). The full gates run at this head once CodeBunny approves.