Repository navigation
feat(panel): add tabs for the Panel - #119
TheGreatAxios merged 7 commits into
Conversation
Add PanelTabs, PanelTab and PanelTabAdd. Tabs select by click, close by the close button, middle-click or Delete, reorder by drag and by Ctrl/Cmd+Shift+Arrow, and move by Arrow, Home and End; a long row keeps the selected tab in view. useTabs owns the open tabs and which one is selected, and useTabDrag reorders without rendering until release. These are separate from the existing Tabs.
fb4e75f to
597ad08
Compare
597ad08 to
a7f3353
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Automated multi-agent review: independent review + second-agent validation; only confirmed items listed. (Severity note: two agents confirmed the top finding; one rated it major, the validator judged it should-fix — recorded as should-fix.)
- should-fix (cross-confirmed by two agents)
src/ui/panel-tabs.tsx:176-188— closing a tab while focus sits inside its pane drops focus todocument.bodyinstead of the tab row. Verified code path:close()only recovers when focus is inside the tab row; the stronger reproduction needs no host code at all — focus a control in the selected pane's contents, click the × on the active tab (itsonMouseDownpreventDefault keeps focus out of the row), the pane unmounts, focus lands on<body>. Middle-click and host-driven closes hit the same path. Smallest fix: capturedocument.activeElementbefore theflushSyncand run the samerefocuswhen it's no longer connected; target the newly selected tab. Permanent regression test: "closing the selected tab while a control inside its pane has focus moves focus to the newly selected tab." - minor (deliberate, unmitigated in docs/stories)
panel-tabs.tsx:424-439— the × affordance isaria-hidden+tabIndex={-1}, so the built-in close is keyboard-only (Delete/Backspace on a focused tab); touch screen-reader users have no reachable close unless the host adds the documented header control — and no story or fixture demonstrates that control. Ship the documented host-side pattern as an example (or offer an optional accessible close slot). - minor (documented deviation)
src/hooks/use-tab-drag.ts:77-79— touch pointers are excluded from drag-to-reorder while plan R40 says "reorder by drag" unqualified; note the tradeoff where the requirement is tracked, alongside the close-control example. - minor — drag-to-reorder announces nothing (
no aria-live/role="status"in the module): a polite live region "Moved to position N" ononMovecommit would cover SR users driving pointer drags. - nits —
paneProps(value)omitsvalue, so callers state it twice and a drift silently shows/hides the wrong pane (use-tabs.ts:178-184); theisProductionsnippet duplicatesuse-controllable-state.ts:15-16verbatim (panel-tabs.tsx:21-22); the header-layout class contract is prose hand-copied into the story and the fixture (panel-tabs.tsx:106-110) — export a preset constant.
a7f3353 to
2dbd57b
Compare
Closing the selected tab with its × or a middle-click while focus sat in its pane dropped focus to the body: neither control takes focus, and only focus in the row was put back. Keep the focused element before the close renders and refocus the newly selected tab when that element left the document with its pane. Tab moves by drag or Ctrl/Cmd+Shift+Arrow were silent to screen readers. A polite status region beside the row now says "Moved to position N of M", worded by an optional movedMessage prop. Export panelTabsHeaderClassName so hosts stop copying the header classes by hand. The story and e2e fixture now show the header "Close tab" button that touch screen-reader users need, since the tab's × is hidden from them. The docs also say why touch does not drag.
useTabDrag wrote the latest onMove into a ref during render. Write it from a layout effect instead, which runs before any pointer event can reach the handler.
2dbd57b to
54d44e9
Compare
|
Thanks for the review. Here's what happened to each point. Fixed1. Closing a tab while focus was inside its pane dropped focus to the page (should-fix)
2. No accessible way to close a tab was shown (minor)
3. Touch doesn't drag (minor)
4. Moving a tab announced nothing (minor)
5. The header classes were copied by hand (nit)
Not changed, and why
Also in this PRA Commits: |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds Panel tabs: select, close, keyboard reorder, and pointer drag.
Findings
-
src/ui/panel-tabs.tsx:234— Dragging a tab that is not selected leaves focus on it after the drop, whiletabIndex={0}stays on the selected tab (panel-tabs.tsx:451). The press focuses the dragged tab, and the release click is swallowed (use-tab-drag.ts:145-149), so it never becomes selected.moveonly runs when the index changes, and then it focuses the moved tab rather than the selected one. Tab and the arrows then start from atabIndex={-1}tab and skip the selected one. After a drag, put focus on the selected tab unless the drag itself changed the selection. -
src/hooks/use-tab-drag.ts:145-148— Escape cancels the reorder but the later mouseup still selects that tab.finish(false)armsswallowClickand removes it on a zero-delay timeout. Escape is not the pointer release, so the timeout runs while the button is still down, and the click on mouseup is a normal select (panel-tabs.tsx:452-454). The samefinish(false)path ispointercancel. Swallow the next click whenever it arrives, not the click that happens to land in this turn.
54d44e9 to
27e0953
Compare
The press that starts a drag focuses the pressed tab, and the drag never selects it, so after a drop focus sat on an unselected tab outside the row's tab stop: the arrows started from it and Delete closed it while another tab was shown. useTabDrag now reports the end of every drag, and PanelTabs sends focus in the row back to the selected tab whether the tab moved or not. Escape cancelled a drag while the button was still down, and the click swallow was disarmed before the release, so letting go selected the tab after all. Swallow the click of that press's own release instead, and stop waiting if another press comes first.
27e0953 to
0b465da
Compare
|
Thanks for the review. Both points are fixed. 1. After dragging a tab that isn't selected, focus stayed on it
2. Releasing a drag cancelled with Escape still selected the tab
Commit: |
useTabs' close always passed an updater to the selection setter, which reports whatever the updater returns. Closing a tab that was not selected still called onValueChange with the unchanged selection. With a controlled value the updater saw the value from the last render, so several closes in one event did not see each other: closing every tab reported a selection that had already closed instead of null. Track the selection the current event has reached in a ref that every commit sets back to the rendered value, and select on close only when the closing tab is that selection. A controlled parent can turn down a plain select without rendering, so that select leaves the ref alone.
The panel tabs story showed "Open panel" only while the panel was closed, so the button left the page in the same commit that mounted the panel. The panel found no opener to record, and closing it, or closing its last tab, sent focus to the body. Keep a toggle in the main area, with aria-expanded and aria-controls, so the panel records it and returns focus to it however it closes.
PanelTabs and PanelTab read `ref` from their props, which only React 19 passes to a function component. On React 18, which the peer range allows, the ref was dropped with a warning, so a host could not reach the row or a tab's root. Wrap both in forwardRef so the refs land on both versions.
0b465da to
7884629
Compare
Round 3 fixesA fresh review pass over the stack turned up the points below; each is fixed in a new commit at the end of this branch, and the stack is rebased onto it.
|
7884629 to
3f4621e
Compare
Superseded by the completed stacked review; blocking fixes were addressed and the rebased stack is being landed.
Add PanelTabs, PanelTab and PanelTabAdd. Tabs select by click, close by the close button, middle-click or Delete, reorder by drag and by Ctrl/Cmd+Shift+Arrow, and move by Arrow, Home and End; a long row keeps the selected tab in view. useTabs owns the open tabs and which one is selected, and useTabDrag reorders without rendering until release. These are separate from the existing Tabs.
Stacked on #118: review only this commit. Part of #94.
Notes for review
items-end bg-muted pt-1.5 pb-0; PanelTabAdd sits beside PanelTabs; spread paneProps(value) on each PanelPane.Verification
bun run typecheckandbun run linton its own (the stack is checked commit by commit).bun run check,bun run build,bun run dep-guard,bun run contrast-testandbun run test:e2e(Playwright specs and the Storybook axe sweep in light and dark) pass.Closes #107