feat(data-table): sticky columns and column settings#382
Conversation
|
Overall this looks good to me, but I have one strong concern about adding The main issue is that the relationship between They are also effectively mutually exclusive, but that is only enforced by a runtime warning and a precedence rule where More broadly, I think row interaction should be expressed through a single API surface rather than split across multiple top-level props. Otherwise users have to learn when to use For this PR, I would prefer to keep If we later want a first-class row navigation API with explicit modes, I think that should be a separate design discussion or follow-up PR. For example, I could imagine something like: type RowInteraction<TRow> =
| { type: "click"; onClick: (row: TRow) => void }
| { type: "link"; href: (row: TRow) => string; primaryColumnId?: string };
useDataTable({
columns,
data,
rowInteraction: { type: "link", href: (row) => `/orders/${row.id}` },
});That would make the modes explicit and avoid having both |
|
Thanks @IzumiSy — agree on all points. I've removed Row interaction is split into a separate design discussion: tailor-inc/platform-planning#1498, seeded with your Also noted from Slack: @sean confirmed localStorage is fine for persistence, so that's settled here. |
There was a problem hiding this comment.
Reviewed the current columns-only scope (noting rowHref was dropped in de74a41), via a local QA pass + adversarial review. Marking as Comment — not requesting changes.
One item I'd flag as a potential DevX blocker: pinning any content column flips the whole table to table-fixed, which silently requires every column to declare a width or it collapses/overlaps — details inline on data-table.tsx, and I'll attach a short video of the collapse. The rest are hardening: an off-by-one in drag-reorder, a width-less-pin UI/table mismatch, an unguarded onClickRow vs custom interactive cells, a positional column-key edge case, and notes on tableId persistence/uniqueness.
col-sticky-table.mp4
Credit where due — the pin offset math, the localStorage parse/validate/reconcile path, and the column-order self-healing are all solid.
| right: buckets.right.filter((k) => k !== dragKey), | ||
| }; | ||
| const target = next[section]; | ||
| target.splice(Math.max(0, Math.min(index, target.length)), 0, dragKey); |
There was a problem hiding this comment.
Downward same-zone reorder lands one slot too far. dropTarget.index is measured against buckets[section], which still contains dragKey, but here we filter dragKey out of the bucket before splicing at that same index. So any drop at/below the item's original slot over-inserts by one — e.g. [A,B,C], drag A onto the lower half of B: the indicator promises [B,A,C] but you get [B,C,A], and the wrong order persists to localStorage when tableId is set. Upward drags are fine.
Fix: with fromIdx = buckets[section].indexOf(dragKey), when fromIdx > -1 && fromIdx < index decrement the insert index by one (cross-zone stays as-is, since fromIdx === -1).
| const target = next[section]; | ||
| target.splice(Math.max(0, Math.min(index, target.length)), 0, dragKey); | ||
| setColumnOrder([...next.left, ...next.scrollable, ...next.right]); | ||
| setPin(dragKey, SECTION_PIN[section]); |
There was a problem hiding this comment.
setPin here (and effectiveSection, which derives a row's zone from pin state alone) never checks width — but the table's computePinLayout ignores a pin without a width. Net effect: dragging a width-less column into Fixed left/right shows it as pinned in this popover while the table renders it in the scrollable region. The only signal is a dev-console warning the end user never sees, and the bogus pin still persists.
Suggest making the settings UI width-aware: treat an unresolvable-width column as scrollable, or refuse the drop into a Fixed zone with visible feedback.
| <Table.Cell | ||
| style={style} | ||
| className={className} | ||
| onClick={(e) => e.stopPropagation()} |
There was a problem hiding this comment.
Now that the row is clickable via onClickRow (the onClick on <Table.Row> just above), any click that bubbles up from a cell fires the row handler. The built-in selection cell (here) and the actions cell below guard against that with e.stopPropagation() — but a consumer who renders a <button> / toggle / inline menu inside a normal column via render gets no such guard, so clicking it also triggers onClickRow. Worth either wrapping cell content so interactive children stop propagation automatically, or documenting that custom interactive cells must call e.stopPropagation().
| actions?: PinPlacement; | ||
| } | ||
|
|
||
| function columnKeyAt<TRow extends Record<string, unknown>>( |
There was a problem hiding this comment.
col.id ?? col.label ?? String(index) is evaluated in two different index spaces: useDataTable / ColumnSettings key off the definition-order column list (where pin/visibility/order are stored — e.g. the col.id ?? col.label ?? String(i) around L902), while columnKeyAt is called at render with the index into the filtered/reordered visible array (L258 / L505 / L742). For any column with an id or label the String(index) branch never fires, so it's masked today — but id and label are both optional (documented for icon-only / render-only columns). For such a column, hiding or reordering a sibling shifts its visible index and its stored pin/visibility silently detaches.
Suggest resolving the key once from a definition-order col -> key Map and threading it through, rather than recomputing positionally at render.
| containerRef={containerRef} | ||
| containerClassName="astw:min-h-0 astw:overflow-auto" | ||
| className={className} | ||
| className={cn(hasColumnPin && "astw:table-fixed", className)} |
There was a problem hiding this comment.
Potential DevX blocker. hasColumnPin (L899, scanning ctx.visibleColumns) flips the whole table to table-fixed whenever any content column is pinned. That imposes a hidden contract: every column must now set an explicit width or it collapses/overlaps once total width exceeds the viewport (table-fixed drops content-sizing for unsized columns). It's infeasible for inferred tables — inferColumns / infer() don't emit a width, and column({ ...infer('field') }) is the idiomatic pattern, so the metadata path we encourage is exactly the one that breaks.
Worth noting the actions menu does NOT trigger this — hasColumnPin scans only content columns, and a lone sticky actions/selection column renders fine in table-auto (offset 0, no width sum to get wrong). A stacked pin needs table-fixed because sticky offsets sum declared widths, which are only authoritative under table-fixed; but table-layout is table-wide, so the pinned stack's requirement leaks onto every unrelated column.
Options: emit a <colgroup> giving width-less columns a min-width / auto share so mixing pinned + unsized degrades gracefully; or avoid the table-wide switch and only enforce fixed widths where stacking actually occurs; at minimum, give inferColumns a default width and console.warn when a pinned table contains width-less columns. Video of the collapse attached to the PR overview.
| // current defaults when there's nothing stored (or the id was cleared). This | ||
| // read does NOT write back, so an existing stored value is never clobbered. | ||
| useEffect(() => { | ||
| if (!tableId) return; |
There was a problem hiding this comment.
Two things worth flagging on tableId-based persistence. (1) When tableId is omitted this hook is still mounted but goes pure in-memory — it silently never reads or writes localStorage, so column state just doesn't persist, with no signal to the developer. That matches the PR description; calling out the silent part. (2) tableId isn't required to be unique and nothing enforces it — it maps straight to a single localStorage key (astw:data-table:v1:<tableId>), so two tables mounted with the same id will compete for and clobber each other's column state.
Suggest a dev-only duplicate-id warning (a mounted-id registry) and/or documenting a namespacing convention (e.g. <route>:<entity>).
…ening Round-2 review follow-ups on the columns PR (#382): - Measure-based sticky offsets: drop the table-wide `table-fixed` switch and compute offsets from measured header-cell widths (PinMeasureContext + an isomorphic layout effect + ResizeObserver). Pinning no longer requires a `width`, so inferColumns/content-sized tables no longer collapse. - Fix off-by-one in same-zone downward drag-reorder in ColumnSettings. - Warn on duplicate `tableId` (a shared localStorage key clobbers layout); docs note on uniqueness. - Balance the row-actions column padding (was 24px right / 8px left). - ColumnSettings drop indicator is now a floating line between rows. Docs, catalogue skill, changeset, and the demo lab updated to match. The onClickRow-vs-custom-interactive-cells item is deferred to the row-interaction issue (tailor-inc/platform-planning#1498). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough pass @interacsean — pushed fixes in ✅ table-fixed DevX blocker (the big one). Removed the table-wide ✅ Off-by-one drag reorder. Same-zone downward drops now decrement the insert index ( ✅ Width-less pin UI/table mismatch. Resolved by the measure-based change above — a width-less column now genuinely pins in both the popover and the table (no more phantom pin). ✅ Duplicate ↪️ onClickRow + custom interactive cells. Pre-existing ➖ Positional column-key edge case. Left as-is: it only affects columns with neither Also, while testing: balanced the row-actions column padding (kebab was pushed left) and reworked the ColumnSettings drop indicator into a floating line between rows. 🙏 |
… navigation Adds three DataTable capabilities driven by the "too many columns / hard to reach the View button" feedback (platform-planning#1489): - Pinned/sticky columns via `pin: "left" | "right"` (selection auto-pins left, row-actions auto-pins right) with a scroll-aware freeze shadow. - `DataTable.ColumnSettings` popover: show/hide, drag-reorder, and pin columns by dragging between Fixed left / Scrollable / Fixed right zones. - Per-user column layout (visibility, order, pinning) persisted to localStorage via a new `tableId` option. - `rowHref` for accessible whole-row navigation (primary cell as a real <Link>), replacing per-row "View" buttons; `onClickRow` kept for non-navigation. Also: `Table.Root` gains an optional `containerRef`; docs, catalogue skill, and a demo lab page updated; changeset added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e dragging The insertion line was a flow element, so inserting it shifted the rows under the cursor, which recomputed the drop target on every reflow — a flicker loop. Draw the indicator as an inset box-shadow on the row instead (no reflow), and remove the inter-row gap so the end-of-section handler no longer fires as the cursor passes between rows. Empty zones get a dashed placeholder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hovering a column's checkbox shows "Hide column" (when visible) or "Show column" (when hidden). Adds showColumn/hideColumn i18n keys (en + ja). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Row interaction is being split out of this PR per review (@IzumiSy): the `rowHref` + `onClickRow` split is a confusing public API, and row interaction should be one explicit surface designed separately. Removes `rowHref` / `primaryColumnId` and the navigation rendering (NavigableRows / <Link> wrap / useNavigate), keeping the existing `onClickRow` untouched. `rowHref` was never released, so this is not a breaking change. Column visibility, sticky/pinned columns, ColumnSettings, tableId persistence, and the Table.Root containerRef are unchanged. Docs, catalogue skill, demo lab page, tests, and changeset updated to columns-only. Row-interaction redesign tracked in tailor-inc/platform-planning#1498. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ening Round-2 review follow-ups on the columns PR (#382): - Measure-based sticky offsets: drop the table-wide `table-fixed` switch and compute offsets from measured header-cell widths (PinMeasureContext + an isomorphic layout effect + ResizeObserver). Pinning no longer requires a `width`, so inferColumns/content-sized tables no longer collapse. - Fix off-by-one in same-zone downward drag-reorder in ColumnSettings. - Warn on duplicate `tableId` (a shared localStorage key clobbers layout); docs note on uniqueness. - Balance the row-actions column padding (was 24px right / 8px left). - ColumnSettings drop indicator is now a floating line between rows. Docs, catalogue skill, changeset, and the demo lab updated to match. The onClickRow-vs-custom-interactive-cells item is deferred to the row-interaction issue (tailor-inc/platform-planning#1498). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
13ed93d to
80aac8a
Compare
There was a problem hiding this comment.
Pull request overview
Adds column-focused UX improvements to the core DataTable component: sticky/pinned columns, a user-facing column settings popover (show/hide, reorder, pin/unpin), and optional per-user persistence via localStorage keyed by a new tableId. Also expands the underlying Table.Root API to expose its scroll container ref, and updates docs/examples/tests accordingly.
Changes:
- Introduces sticky/pinned column rendering (including selection/actions auto-pinning and scroll-aware “freeze shadow”) and pin-related APIs (
Column.pin,pinnedColumns,setPin). - Adds persisted column layout support via
tableId(visibility/order/pinning stored inlocalStorage) and new ordering APIs (columnOrder,moveColumn,setColumnOrder). - Adds
DataTable.ColumnSettingsUI + supporting docs, examples (DataTable Lab), and test coverage.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/components/table.tsx | Adds containerRef to Table.Root so consumers can observe the scroll container. |
| packages/core/src/components/data-table/use-persistent-column-state.ts | New SSR-safe localStorage persistence hook for column layout state. |
| packages/core/src/components/data-table/use-data-table.ts | Replaces in-memory hidden-column state with persisted visibility/order/pinning and exposes new APIs. |
| packages/core/src/components/data-table/use-data-table.test.ts | Adds tests for column ordering/pinning and persistence behavior. |
| packages/core/src/components/data-table/types.ts | Adds Column.pin, UseDataTableOptions.tableId, and new return fields for order/pinning. |
| packages/core/src/components/data-table/i18n.ts | Adds labels for column settings UI. |
| packages/core/src/components/data-table/data-table.tsx | Implements sticky column layout, width measurement, scroll-edge shadowing, and exposes DataTable.ColumnSettings. |
| packages/core/src/components/data-table/data-table.test.tsx | Updates truncate assertions and adds pinned-column behavior tests. |
| packages/core/src/components/data-table/data-table-context.tsx | Extends context with order/pinning APIs. |
| packages/core/src/components/data-table/column-settings.tsx | New DataTable.ColumnSettings popover UI for show/hide, reorder, and pin/unpin. |
| packages/core/src/components/data-table/column-settings.test.tsx | Adds UI-level tests for column settings behavior (toggle, drag/drop pinning, show/hide all). |
| examples/vite-app/src/routes.generated.ts | Adds route typing for the new DataTable Lab page. |
| examples/vite-app/src/pages/data-table-lab/page.tsx | Adds a demo “DataTable Lab” page exercising the new column features. |
| examples/vite-app/src/App.tsx | Adds sidebar navigation entry for DataTable Lab. |
| docs/components/data-table.md | Documents pinning, column settings, and tableId persistence. |
| catalogue/src/pattern/list/dense-scan/PATTERN.md | Updates pattern guidance to recommend pinning + column settings for wide lists. |
| catalogue/src/fundamental/components.md | Updates DataTable catalogue documentation with pinning and column settings usage. |
| .changeset/data-table-columns-and-row-nav.md | Minor changeset describing the new DataTable column features and Table.Root.containerRef. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Hydrate from storage on mount / when the table id changes. Falls back to the | ||
| // current defaults when there's nothing stored (or the id was cleared). This | ||
| // read does NOT write back, so an existing stored value is never clobbered. | ||
| useEffect(() => { | ||
| if (!tableId) return; | ||
| setStateRaw(readState(tableId) ?? defaultsRef.current); | ||
| }, [tableId]); |
| const moveColumn = useCallback( | ||
| (key: string, toIndex: number) => { | ||
| setPersisted((prev) => { | ||
| const order = [...columnOrder]; | ||
| const from = order.indexOf(key); | ||
| if (from === -1) return prev; | ||
| const clamped = Math.max(0, Math.min(toIndex, order.length - 1)); | ||
| order.splice(from, 1); | ||
| order.splice(clamped, 0, key); | ||
| return { ...prev, order }; | ||
| }); | ||
| }, | ||
| [setPersisted, columnOrder], | ||
| ); |
- usePersistentColumnState: reset to defaults when `tableId` is cleared instead of keeping the previous table's persisted layout (in-memory mode shouldn't inherit a prior id's state). - moveColumn: reconcile column order from `prev` inside the state updater (via a shared `reconcileColumnOrder` helper) rather than a captured `columnOrder`, so reorder calls batched before a re-render compose instead of clobbering. Adds regression tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the Copilot review comments in
Both are low-severity edge cases (the built-in drag UI uses |
Collapse the two footer buttons into a single right-aligned button whose label and action follow visibility state: "Hide all" when every column is visible, "Show all" otherwise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Column controls for
DataTable, from the "too many columns / constant scroll-right" feedback in platform-planning#1489.pin: "left" | "right"on aColumn(requireswidth). Selection auto-pins left, row-actions auto-pins right, with a scroll-aware freeze shadow that appears once content scrolls under the edge.DataTable.ColumnSettings— a "Columns" toolbar popover with three drop zones (Fixed left / Scrollable / Fixed right): show/hide via checkboxes, drag to reorder, and drag between zones to pin/unpin.tableIdto persist each user's visibility/order/pinning tolocalStorage. Per-user preference, not URL. Omit for in-memory only.Table.Rootgains an optionalcontainerRef.Docs (
docs/components/data-table.md), the catalogue skill (components.md/list-dense-scan→ regeneratedapp-shell-patterns), a demo DataTable Lab page, tests, and aminorchangeset are included.Persistence
Resolved per @sean on Slack — localStorage is fine for now (per-device layout is acceptable; a synced user-settings store would be more complexity for little benefit).
Try it
pnpm dev→ DataTable Lab in the sidebar: open Columns to show/hide, drag to reorder, drag between zones to pin; scroll horizontally to see pinned columns + freeze shadow.🤖 Generated with Claude Code