From 65042da62194811fc07c67788f123ec4cbf42b4a Mon Sep 17 00:00:00 2001 From: Eddie Date: Thu, 18 Jun 2026 13:28:32 -0400 Subject: [PATCH 1/9] fix(datatable): improve sortable header accessibility Co-authored-by: Codex --- src/DataTable/TableHeaderCell.tsx | 30 ++++++-- src/DataTable/index.scss | 16 +++++ src/DataTable/tests/TableHeaderCell.test.jsx | 74 ++++++++++++++++++-- 3 files changed, 107 insertions(+), 13 deletions(-) diff --git a/src/DataTable/TableHeaderCell.tsx b/src/DataTable/TableHeaderCell.tsx index a314dadcd85..a88bd5d817c 100644 --- a/src/DataTable/TableHeaderCell.tsx +++ b/src/DataTable/TableHeaderCell.tsx @@ -31,7 +31,7 @@ interface TableHeaderCellProps { render: (type: 'Header') => React.ReactNode; /** Indicates whether the column is sorted in descending order */ isSortedDesc?: boolean; - /** Gets props related to sorting that will be passed to th */ + /** Gets props related to sorting that will be passed to the sort button */ getSortByToggleProps?: (...args: any[]) => Record; /** Indicates whether a column is sortable */ canSort?: boolean; @@ -49,13 +49,31 @@ function TableHeaderCell({ headerClassName, }: TableHeaderCellProps) { const toggleProps = canSort && getSortByToggleProps ? getSortByToggleProps() : {}; + const ariaSort: React.AriaAttributes['aria-sort'] = isSorted + ? (isSortedDesc ? 'descending' : 'ascending') + : undefined; + const headerContentClassName = classNames('pgn__data-table-header-content', headerClassName); return ( - - - {render('Header')} - {canSort && } - + + {canSort ? ( + + ) : ( + + {render('Header')} + + )} ); } diff --git a/src/DataTable/index.scss b/src/DataTable/index.scss index 4292f5bde38..e3e4ebe2648 100644 --- a/src/DataTable/index.scss +++ b/src/DataTable/index.scss @@ -148,6 +148,22 @@ text-align: start; } + .pgn__data-table-header-content { + display: flex; + align-items: center; + } + + .pgn__data-table-sort-button { + background: transparent; + border: 0; + color: inherit; + cursor: pointer; + font: inherit; + padding: 0; + text-align: inherit; + width: 100%; + } + td { padding: var(--pgn-spacing-data-table-padding-cell); line-height: 24px; diff --git a/src/DataTable/tests/TableHeaderCell.test.jsx b/src/DataTable/tests/TableHeaderCell.test.jsx index 89ac45f1841..944907ae9c3 100644 --- a/src/DataTable/tests/TableHeaderCell.test.jsx +++ b/src/DataTable/tests/TableHeaderCell.test.jsx @@ -1,9 +1,10 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import TableHeaderCell from '../TableHeaderCell'; -const sortByToggleProps = { foo: 'bar' }; +const sortByToggleProps = { 'data-sort-prop': 'bar' }; const props = { getHeaderProps: () => ({ className: 'red' }), render: () => 'Title', @@ -20,21 +21,35 @@ function FakeTable({ ...rest }) { describe('', () => { describe('unsorted', () => { - render(); - const cell = screen.getByRole('columnheader'); - const innerCell = cell.firstChild; - it('renders a table header cell', () => { + render(); + const cell = screen.getByRole('columnheader'); expect(cell).toBeInTheDocument(); }); it('adds props to the cell', () => { + render(); + const cell = screen.getByRole('columnheader'); expect(cell.className).toBe('red'); }); + it('adds column scope to the cell', () => { + render(); + const cell = screen.getByRole('columnheader'); + expect(cell).toHaveAttribute('scope', 'col'); + }); + it('adds the headerClassName to inner span', () => { + render(); + const cell = screen.getByRole('columnheader'); + const innerCell = cell.firstChild; expect(innerCell.className).toContain(props.headerClassName); }); + + it('does not render a button for non-sortable headers', () => { + render(); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + }); }); describe('with sorting', () => { @@ -56,10 +71,55 @@ describe('', () => { expect(sortIndicator).toBeInTheDocument(); }); - it('adds the toggle props to the header props if toggle props are available', () => { + it('renders the sort toggle as a button', () => { + render(); + expect(screen.getByRole('button', { name: 'Title' })).toBeInTheDocument(); + }); + + it('adds ascending sort state to the header cell when sorted ascending', () => { + render(); + const cell = screen.getByRole('columnheader'); + expect(cell).toHaveAttribute('aria-sort', 'ascending'); + }); + + it('adds descending sort state to the header cell when sorted descending', () => { + render(); + const cell = screen.getByRole('columnheader'); + expect(cell).toHaveAttribute('aria-sort', 'descending'); + }); + + it('does not add inactive sort state to unsorted header cells', () => { + render(); + const cell = screen.getByRole('columnheader'); + expect(cell).not.toHaveAttribute('aria-sort'); + }); + + it('adds the toggle props to the sort button if toggle props are available', () => { + render(); + const button = screen.getByRole('button', { name: 'Title' }); + expect(button).toHaveAttribute('data-sort-prop', sortByToggleProps['data-sort-prop']); + }); + + it('does not pass toggle props to the header props', () => { const headerPropsSpy = jest.fn().mockReturnValueOnce({}); render(); - expect(headerPropsSpy).toHaveBeenCalledWith(sortByToggleProps); + expect(headerPropsSpy).toHaveBeenCalledWith(); + }); + + it('makes sortable headers keyboard reachable and operable', async () => { + const user = userEvent.setup(); + const handleSort = jest.fn(); + render( ({ onClick: handleSort })} />); + const button = screen.getByRole('button', { name: 'Title' }); + + await user.tab(); + expect(button).toHaveFocus(); + + await user.keyboard('{Enter}'); + expect(handleSort).toHaveBeenCalledTimes(1); + + await user.keyboard(' '); + expect(handleSort).toHaveBeenCalledTimes(2); }); }); }); From 2da6161837e418cffc6ebe2f4e844ec3448ea69f Mon Sep 17 00:00:00 2001 From: Eddie Date: Thu, 18 Jun 2026 13:42:37 -0400 Subject: [PATCH 2/9] fix(datatable): avoid nested aria sort ternary Co-authored-by: Codex --- src/DataTable/TableHeaderCell.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/DataTable/TableHeaderCell.tsx b/src/DataTable/TableHeaderCell.tsx index a88bd5d817c..0d606590d0a 100644 --- a/src/DataTable/TableHeaderCell.tsx +++ b/src/DataTable/TableHeaderCell.tsx @@ -49,9 +49,10 @@ function TableHeaderCell({ headerClassName, }: TableHeaderCellProps) { const toggleProps = canSort && getSortByToggleProps ? getSortByToggleProps() : {}; - const ariaSort: React.AriaAttributes['aria-sort'] = isSorted - ? (isSortedDesc ? 'descending' : 'ascending') - : undefined; + let ariaSort: React.AriaAttributes['aria-sort']; + if (isSorted) { + ariaSort = isSortedDesc ? 'descending' : 'ascending'; + } const headerContentClassName = classNames('pgn__data-table-header-content', headerClassName); return ( From 597f22428c72bdd8e3d40ae58a3815c5dd6f52c1 Mon Sep 17 00:00:00 2001 From: Eddie Date: Thu, 18 Jun 2026 13:59:27 -0400 Subject: [PATCH 3/9] fix(datatable): preserve sortable header hit target Co-authored-by: Codex --- src/DataTable/TableHeaderCell.tsx | 11 ++++++++++- src/DataTable/index.scss | 7 ++++++- src/DataTable/tests/TableHeaderCell.test.jsx | 14 ++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/DataTable/TableHeaderCell.tsx b/src/DataTable/TableHeaderCell.tsx index 0d606590d0a..8a5ebb6825c 100644 --- a/src/DataTable/TableHeaderCell.tsx +++ b/src/DataTable/TableHeaderCell.tsx @@ -48,6 +48,7 @@ function TableHeaderCell({ isSortedDesc = false, headerClassName, }: TableHeaderCellProps) { + const headerProps = getHeaderProps(); const toggleProps = canSort && getSortByToggleProps ? getSortByToggleProps() : {}; let ariaSort: React.AriaAttributes['aria-sort']; if (isSorted) { @@ -56,7 +57,15 @@ function TableHeaderCell({ const headerContentClassName = classNames('pgn__data-table-header-content', headerClassName); return ( - + {canSort ? ( ) : ( diff --git a/src/DataTable/tests/TableHeaderCell.test.jsx b/src/DataTable/tests/TableHeaderCell.test.jsx index 29eefaf5639..7b656ec9343 100644 --- a/src/DataTable/tests/TableHeaderCell.test.jsx +++ b/src/DataTable/tests/TableHeaderCell.test.jsx @@ -90,6 +90,13 @@ describe('', () => { expect(button).toHaveClass('pgn__data-table-sort-button'); }); + it('adds headerClassName to sortable header content', () => { + render(); + const button = screen.getByRole('button', { name: 'Title' }); + expect(button).not.toHaveClass(props.headerClassName); + expect(button.firstChild).toHaveClass(props.headerClassName); + }); + it('adds ascending sort state to the header cell when sorted ascending', () => { render(); const cell = screen.getByRole('columnheader'); From c2cc03cd16c1fcb5d8db14d036da2d8f318efdc8 Mon Sep 17 00:00:00 2001 From: Eddie Date: Thu, 18 Jun 2026 18:34:45 -0400 Subject: [PATCH 9/9] fix(datatable): avoid header row key spreads Co-authored-by: Codex --- src/DataTable/TableHeaderRow.jsx | 26 +++++++++------ src/DataTable/tests/TableHeaderRow.test.jsx | 35 ++++++++++++++++++--- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/DataTable/TableHeaderRow.jsx b/src/DataTable/TableHeaderRow.jsx index 04188ddec98..7aba3d64f56 100644 --- a/src/DataTable/TableHeaderRow.jsx +++ b/src/DataTable/TableHeaderRow.jsx @@ -5,13 +5,21 @@ import TableHeaderCell from './TableHeaderCell'; function TableHeaderRow({ headerGroups }) { return ( - {headerGroups.map(headerGroup => ( - - {headerGroup.headers.map(column => ( - - ))} - - ))} + {headerGroups.map((headerGroup) => { + const { key: headerGroupKey, ...headerGroupProps } = headerGroup.getHeaderGroupProps(); + + return ( + + {headerGroup.headers.map((column) => { + const { key: headerKey } = column.getHeaderProps(); + + return ( + + ); + })} + + ); + })} ); } @@ -19,10 +27,10 @@ function TableHeaderRow({ headerGroups }) { TableHeaderRow.propTypes = { headerGroups: PropTypes.arrayOf(PropTypes.shape({ headers: PropTypes.arrayOf(PropTypes.shape({ - /** Props for the TableHeaderCell component. Must include a key */ + /** Props for the TableHeaderCell component. Must include a React key */ getHeaderProps: PropTypes.func.isRequired, })).isRequired, - /** Returns props for the header tr element */ + /** Returns props for the header tr element. Must include a React key */ getHeaderGroupProps: PropTypes.func.isRequired, })).isRequired, }; diff --git a/src/DataTable/tests/TableHeaderRow.test.jsx b/src/DataTable/tests/TableHeaderRow.test.jsx index feccb65511e..33e7a040d31 100644 --- a/src/DataTable/tests/TableHeaderRow.test.jsx +++ b/src/DataTable/tests/TableHeaderRow.test.jsx @@ -31,24 +31,51 @@ const props = { }], }; -describe('', () => { +function renderTableHeaderRow() { render(
); - const head = screen.getByRole('rowgroup'); - const row = screen.getByRole('row'); - const cells = screen.getAllByRole('columnheader'); +} +describe('', () => { it('renders a table head and row', () => { + renderTableHeaderRow(); + + const head = screen.getByRole('rowgroup'); + const row = screen.getByRole('row'); + expect(head).toBeInTheDocument(); expect(row).toBeInTheDocument(); }); it('adds props to the row', () => { + renderTableHeaderRow(); + + const row = screen.getByRole('row'); + expect(row.className).toEqual('red'); }); it('renders cells', () => { + renderTableHeaderRow(); + + const cells = screen.getAllByRole('columnheader'); + expect(cells.length).toEqual(2); expect(cells[0]).toHaveTextContent(header1Name); expect(cells[1]).toHaveTextContent(header2Name); }); + + it('does not spread React keys from react-table prop getters', () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); + + try { + renderTableHeaderRow(); + + const keySpreadWarning = consoleError.mock.calls.some(([message]) => ( + String(message).includes('A props object containing a "key" prop') + )); + expect(keySpreadWarning).toEqual(false); + } finally { + consoleError.mockRestore(); + } + }); });