Repository navigation
DataTable: Add current-page row selection - #8415
Conversation
Add controlled and uncontrolled selection for flat and grouped tables with accessible selection controls and regression coverage.\n\nRefs github/primer#6718 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: bacc392 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
integration-tests: recommended
|
rickyzhangca
added
the
update snapshots
github-actions
Bot
removed
the
update snapshots
github-actions
Bot
added
integration-tests: failing
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A moderate accessibility issue remains unresolved in row checkbox fallback naming.
Review tier: Lite
Findings: None
What changed in this PR
Adds opt-in current-page row selection to DataTable, including controlled state, grouped rows, accessibility support, documentation, and tests.
Changes:
- Added stable-ID selection APIs and select-all behavior.
- Added composable selection components.
- Added examples, accessibility documentation, browser tests, and a minor changeset.
| File | Summary |
|---|---|
packages/react/src/experimental/index.ts |
Exports selection prop types. |
packages/react/src/DataTable/useTable.ts |
Manages selection state and derived row status. |
packages/react/src/DataTable/Table.tsx |
Adds composable selection parts. |
packages/react/src/DataTable/Table.module.css |
Styles selection controls. |
packages/react/src/DataTable/Table.features.stories.tsx |
Demonstrates composed selection. |
packages/react/src/DataTable/index.ts |
Exports selection components and types. |
packages/react/src/DataTable/DataTable.types.test.tsx |
Validates selection typings. |
packages/react/src/DataTable/DataTable.tsx |
Integrates selection into DataTable. |
packages/react/src/DataTable/DataTable.features.stories.tsx |
Adds selection examples. |
packages/react/src/DataTable/DataTable.docs.json |
Documents selection APIs. |
packages/react/src/DataTable/DataTable.accessibility.md |
Documents selection semantics. |
packages/react/src/DataTable/__tests__/DataTableSelection.test.tsx |
Tests selection behavior and hydration. |
packages/react/src/__tests__/__snapshots__/exports.test.ts.snap |
Updates export snapshots. |
e2e/components/Table.test.ts |
Tests composed-selection interactions and accessibility. |
e2e/components/DataTable.test.ts |
Tests DataTable selection scenarios. |
.changeset/selectable-data-table-rows.md |
Declares the minor release. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
joshblack
left a comment
joshblack
left a comment
There was a problem hiding this comment.
Looking great so far! Thanks for putting this up. The config-based API makes a lot of sense to me 💯 Left a couple of comments/questions and then also had some broader things:
- One though, do you think it'd be nice for data table to start to break down the sections more? We don't really do this for other components but I bet for data table it's worth it
- For select all, are we including the "select all" bit or is that in the batch actions stuff you mentioned is coming later on? It'd be nice to include that with pagination because without it the example is kind of a bummer 😞
- I was curious about some of the demo controls (like the buttons to refresh pages), what are they for? Would they be better as dev stories?
- For the selectable standalone and grouped repo story I was a little confused since I couldn't select the standalone one, what is the intended behavior?
Thanks again for putting this up! Hope these all make sense, can't wait to hear what you think 👀
totally, i also found
sorry I didn't follow this, do you mean the ability to select all and do batch actions? this pr will enable selecting all, later on we add bulk actions
they are for demoing certain behaviours, for example the refresh button demos the selection will persist between data refreshes
yea that can be confusing, remove that disabled row |
Address review feedback with semantic selection props and a shared row ID type. Simplify examples and browser coverage without changing current-page selection ownership. Refs github/primer#6718 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
removed
the
update snapshots
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rickyzhangca
added
the
update snapshots
github-actions
Bot
removed
the
update snapshots
|
@joshblack a very gentle bump!! |
joshblack
left a comment
joshblack
left a comment
There was a problem hiding this comment.
Looking great! Just had some questions / comments
| aria-label="Select rows" | ||
| aria-description={ | ||
| selectableRows.length > 0 | ||
| ? `Select all ${selectableRows.length} ${selectableRows.length === 1 ? 'row' : 'rows'}` | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
@rickyzhangca do you happen to know if these are always required a11y-wise?
There was a problem hiding this comment.
yea we got a11y tickets previously on checkboxes https://github.com/github/accessibility/issues/10271
There was a problem hiding this comment.
aria-label probably can be removed though since aria-description already does the job, removed!
| return ( | ||
| <TableHeader | ||
| {...rest} | ||
| className={clsx('TableSelectionHeader', className, classes.TableSelectionHeader)} |
There was a problem hiding this comment.
For the new components, feel free to leave off the global classes (e.g. TableSelectionHeader in this case). These are just in the file currently for backwards-compat but we'll remove them in the future.
| const id = useId() | ||
| const titleId = `${id}-title` | ||
| const selectionColumnId = `${id}-selection` | ||
| const nameColumnId = `${id}-name` | ||
| const visibilityColumnId = `${id}-visibility` | ||
| const [selectedRows, setSelectedRows] = useState<ReadonlySet<DataTableRowId>>(() => new Set([2])) | ||
| const selectableRows = groups.flatMap(group => group.rows).filter(row => row.id !== 1) | ||
| const selectedCount = selectableRows.filter(row => selectedRows.has(row.id)).length | ||
| const allSelected = selectableRows.length > 0 && selectedCount === selectableRows.length |
There was a problem hiding this comment.
For these Table-specific stories, how would you feel about using useTable with them? I wasn't sure but could help reduce duplication stuff. Also would get if it feels like re-creating data table though lol.
There was a problem hiding this comment.
haha yea it'll be kinda like another small data table
There was a problem hiding this comment.
these ones are there just because I wanted to demo "BYOD", composing a table with table components only, also useTable isn't public for others to use anyway, so I'd lean towards keeping these examples small and minimal
| selectionId: rowId, | ||
| selectionLabelledBy: rowHeaderIds.length > 0 ? rowHeaderIds.join(' ') : undefined, | ||
| selected: selectable && selectedRows.has(rowId), | ||
| selectable, |
There was a problem hiding this comment.
Did you notice any difference between calculating these and making them lazy either behind a getter or explicit get* function? I always wondered about this with tanstack's table so was curious if you noticed anything 👀
There was a problem hiding this comment.
i viewed it as a precaution, lemme test
There was a problem hiding this comment.
2 rounds each, without DOM
| # of rows | data | current | getter | get*() |
|---|---|---|---|---|
| 10 | Flat | 0.00170 / 0.00170 | 0.00325 / 0.00325 | 0.00150 / 0.00145 |
| 10 | Grouped | 0.00170 / 0.00170 | 0.00325 / 0.00325 | 0.00150 / 0.00150 |
| 10 | Mixed | 0.00170 / 0.00170 | 0.00330 / 0.00330 | 0.00150 / 0.00150 |
| 100 | Flat | 0.0160 / 0.0160 | 0.0325 / 0.0325 | 0.0140 / 0.0140 |
| 100 | Grouped | 0.0165 / 0.0170 | 0.0330 / 0.0340 | 0.0140 / 0.0140 |
| 100 | Mixed | 0.0160 / 0.0165 | 0.0330 / 0.0335 | 0.0145 / 0.0140 |
| 1,000 | Flat | 0.160 / 0.160 | 0.335 / 0.335 | 0.140 / 0.140 |
| 1,000 | Grouped | 0.160 / 0.165 | 0.335 / 0.340 | 0.140 / 0.145 |
| 1,000 | Mixed | 0.165 / 0.165 | 0.340 / 0.340 | 0.145 / 0.145 |
with DOM
| # of rows | data | current | getter | get*() |
|---|---|---|---|---|
| 10 | Flat | 0.07 / 0.08 | 0.07 / 0.08 | 0.07 / 0.07 |
| 10 | Grouped | 0.07 / 0.08 | 0.08 / 0.08 | 0.08 / 0.07 |
| 10 | Mixed | 0.07 / 0.07 | 0.08 / 0.08 | 0.07 / 0.07 |
| 100 | Flat | 0.48 / 0.48 | 0.50 / 0.51 | 0.50 / 0.50 |
| 100 | Grouped | 0.54 / 0.54 | 0.56 / 0.57 | 0.55 / 0.55 |
| 100 | Mixed | 0.53 / 0.54 | 0.57 / 0.57 | 0.54 / 0.54 |
| 1,000 | Flat | 5.50 / 5.50 | 5.85 / 5.85 | 5.60 / 5.55 |
| 1,000 | Grouped | 6.30 / 6.30 | 6.40 / 6.35 | 6.40 / 6.35 |
| 1,000 | Mixed | 6.15 / 6.30 | 6.50 / 6.50 | 6.60 / 6.65 |
don't think it's significant enough difference, we could keep it as is?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Integration test results from github/github-ui PR:
All checks passed! |
github-actions
Bot
added
integration-tests: passing
Refs github/primer#6718
Implements the row-selection stage for DataTable, building on the grouping support merged in #8386. This PR targets
maindirectly and does not close the tracking issue: bulk actions remain a follow-up.CleanShot.2026-09-14.at.10.28.16.AM.mp4
Changelog
New
rowSelection,selectedRows,defaultSelectedRows,onSelectionChange, andisRowSelectableprops, using the existinggetRowIdfor stable identity.Table.SelectionHeaderandTable.RowSelectionparts using Primer Checkbox.Changed
Removed
Rollout strategy
Includes a minor changeset. Selection is opt-in; existing consumers do not need to migrate.
Testing & Reviewing
State ownership and scope
selectedRows/defaultSelectedRowsacceptReadonlySet<string | number>;onSelectionChangereceives{selectedRows: Set<string | number>}.Automated validation on this HEAD
Reviewer focus / outstanding validation
WithRowSelection,WithGroupedRowSelection, andWithMixedRowSelectionunder DataTable / Features.Kept as draft pending review and manual accessibility validation.