diff --git a/.changeset/strong-onions-work.md b/.changeset/strong-onions-work.md new file mode 100644 index 000000000..a1110cdab --- /dev/null +++ b/.changeset/strong-onions-work.md @@ -0,0 +1,5 @@ +--- +"@atomicjolt/atomic-elements": patch +--- + +fix grouped header alignment and duplication diff --git a/packages/atomic-elements/src/components/Content/Table/Table.spec.tsx b/packages/atomic-elements/src/components/Content/Table/Table.spec.tsx index 27ad17f15..e2efda5d9 100644 --- a/packages/atomic-elements/src/components/Content/Table/Table.spec.tsx +++ b/packages/atomic-elements/src/components/Content/Table/Table.spec.tsx @@ -1,6 +1,6 @@ import { useState } from "react"; import { fireEvent, render, screen } from "@testing-library/react"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { Table } from "."; import { TableProps, LoadingProps } from "./Table.types"; import { Button } from "@components/Buttons/Button"; @@ -134,6 +134,131 @@ describe("Table", () => { 10000 ); + it("keeps grouped header rows aligned with their leaf columns when a nested sub-group is toggled", () => { + // Regression test for #267: buildHeaderRows names each placeholder + // after whichever real column borders it (the next column for a + // mid-row gap, the last-placed column for a trailing one) rather than + // by its own position. A column with a gap on both sides - here, + // "Milestones" sits between the gap left by "Lead" and the gap left by + // "Owner" - gets two placeholders sharing that one name. Without a + // position-based key (see TableHeaderRowCells in TableHeader.tsx), React + // can't tell the two placeholders apart, so a stale cell survives every + // toggle and the header row above them keeps growing wider than the + // columns underneath it instead of tracking the current leaf count. + function Harness() { + const [nested, setNested] = useState(true); + return ( +
+ + + + + + + City + + State + + Lead + + + {nested ? ( + + M1 + M2 + + ) : ( + M1 + )} + Owner + + + + + Austin + TX + Jane + Kickoff + {nested && Beta} + Priya + + +
+
+ ); + } + + const { container, getByText } = render(); + const button = getByText("Toggle"); + + function headerRowWidths() { + return Array.from(container.querySelectorAll("thead tr")).map((row) => + Array.from(row.querySelectorAll("th")).reduce( + (sum, th) => sum + Number(th.getAttribute("colspan") || 1), + 0 + ) + ); + } + + // Starts nested (6 leaves); each click flips between 5 leaves + // (collapsed) and 6 (nested). Every header row should always sum to + // whichever is current - never more, and never a leftover from before. + for (let i = 0; i < 6; i++) { + fireEvent.click(button); + const expectedWidth = i % 2 === 0 ? 5 : 6; + headerRowWidths().forEach((width) => expect(width).toBe(expectedWidth)); + } + }); + + it("does not emit a duplicate-key warning for a column with placeholder gaps on both sides", () => { + // The duplicate placeholder key from #267 isn't toggle-induced - it's a + // property of the row's shape, present from the very first render. + const errors: unknown[][] = []; + const spy = vi.spyOn(console, "error").mockImplementation((...args) => { + errors.push(args); + }); + + render( + + + + + + City + + State + + Lead + + + + M1 + M2 + + Owner + + + + + Austin + TX + Jane + Kickoff + Beta + Priya + + +
+ ); + + spy.mockRestore(); + + const keyWarnings = errors.filter((args) => + args.some((arg) => typeof arg === "string" && /same key/i.test(arg)) + ); + expect(keyWarnings).toHaveLength(0); + }); + it("should renderEmpty when there is no rows", () => { render( diff --git a/packages/atomic-elements/src/components/Content/Table/components/TableHeader.tsx b/packages/atomic-elements/src/components/Content/Table/components/TableHeader.tsx index 70ad5b7ef..c7318c85d 100644 --- a/packages/atomic-elements/src/components/Content/Table/components/TableHeader.tsx +++ b/packages/atomic-elements/src/components/Content/Table/components/TableHeader.tsx @@ -3,7 +3,11 @@ import { GridNode } from "@react-types/grid"; import { Node } from "react-stately"; import { useTableSelectAllCheckbox } from "@react-aria/table"; import { useRenderProps } from "@hooks/useRenderProps"; -import { Collection, createBranchComponent, useCachedChildren } from "@react-aria/collections"; +import { + Collection, + createBranchComponent, + useCachedChildren, +} from "@react-aria/collections"; import { CheckBox, CheckBoxContext } from "@components/Inputs/Checkbox"; import { DEFAULT_SLOT } from "@hooks/useSlottedContext"; @@ -31,7 +35,21 @@ function TableHeaderRowCells(props: TableHeaderRowCellsProps) { items: state.collection.getChildren!(row.key), children: (node: GridNode) => node.type === "placeholder" ? ( - + // buildHeaderRows names a placeholder after whichever real column + // borders it (the next column for a mid-row gap, the last-placed + // column for a trailing one), not after its own position. A column + // with a gap on both sides - e.g. a group flanked by two shorter + // siblings - produces two placeholders sharing that one name, which + // duplicates/loses cells across renders. Row + index is + // always unique per row, so key off that instead. Use `id`, not + // `key`: useCachedChildren clones this element and keys it off + // `props.id` (falling back to the node's own colliding `.key` + // otherwise - see useCachedChildren.ts), so a plain `key` prop here + // would be discarded. + ) : ( node.render!(node) ), @@ -40,7 +58,17 @@ function TableHeaderRowCells(props: TableHeaderRowCellsProps) { return <>{cells}; } -function TableColumnPlaceholder({ node }: { node: GridNode }) { +interface TableColumnPlaceHolderProps { + id: string; + node: GridNode; +} + +function TableColumnPlaceholder({ + // Intentionally ignore the id as it's just used to override the + // react-aria key management, it doesn't need to make it into the DOM + id: _id, + node, +}: TableColumnPlaceHolderProps) { return (