feat(ui): remove DatagridCheckboxCell component - #1886
Conversation
- don’t render Checkbox automatically, but accept children - always center vertically Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
allow for overriding the vertical alignment set on the parent Datagrid Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
- render DataGrid column context - show verticalAlignment - demonstrate no-wrap - demonstrate truncation with ellipsis Signed-off-by: Franz Heidl <franz.heidl@sap.com>
… parent Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
🦋 Changeset detectedLatest commit: 32bd9ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
|
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
There was a problem hiding this comment.
Pull request overview
This PR finalizes and streamlines DataGridCheckboxCell by turning it into a layout-only container for aligning a consumer-provided Checkbox child, introduces a per-cell verticalAlignment override on DataGridCell, and updates tests/stories to reflect the new APIs and recommended usage patterns.
Changes:
- Refactors
DataGridCheckboxCellto acceptchildrenand removes Checkbox-specific props/behavior. - Adds
verticalAlignmenttoDataGridCellto override the parentDataGrid’scellVerticalAlignmenton a per-cell basis. - Updates unit tests and Storybook stories (including
DataGridHeader’s FullyFeatured story) to demonstrate the new patterns.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/components/DataGridCheckboxCell/DataGridCheckboxCell.component.tsx | Refactors checkbox cell to be child-driven and enforces vertical centering behavior. |
| packages/ui-components/src/components/DataGridCheckboxCell/DataGridCheckboxCell.test.tsx | Updates tests to validate new classnames/children behavior and centering. |
| packages/ui-components/src/components/DataGridCheckboxCell/DataGridCheckboxCell.stories.tsx | Rewrites stories to demonstrate wiring selection state on the child Checkbox. |
| packages/ui-components/src/components/DataGridCell/DataGridCell.component.tsx | Adds verticalAlignment prop and applies it as an override to grid context alignment. |
| packages/ui-components/src/components/DataGridCell/DataGridCell.test.tsx | Adds coverage for inherited vs overridden vertical alignment and other props. |
| packages/ui-components/src/components/DataGridCell/DataGridCell.stories.tsx | Updates stories to show grid context, nowrap/ellipsis patterns, and alignment overrides. |
| packages/ui-components/src/components/DataGrid/DataGridHeader.stories.tsx | Updates FullyFeatured story to use the new DataGridCheckboxCell API with a child Checkbox. |
Suppressed comments (3)
packages/ui-components/src/components/DataGridCheckboxCell/DataGridCheckboxCell.stories.tsx:99
- This disabled
Checkboxhas no accessible name. Add anaria-label(or a visible label) so assistive tech users can understand what the control represents even when disabled.
<DataGridCheckboxCell>
<Checkbox disabled />
</DataGridCheckboxCell>
packages/ui-components/src/components/DataGridCheckboxCell/DataGridCheckboxCell.stories.tsx:128
- These row
Checkboxcontrols have no accessible name. Add anaria-labelper item (or a visible label) so the example is accessible and demonstrates best-practice usage.
<DataGridCheckboxCell>
<Checkbox />
</DataGridCheckboxCell>
packages/ui-components/src/components/DataGrid/DataGridHeader.stories.tsx:380
- This second table instance also renders row selection
Checkboxcomponents without an accessible name. Add anaria-label(or visible label) per row to keep the example accessible.
<DataGridCheckboxCell>
<Checkbox
checked={!!selected[s.id]}
onChange={(e) => setSelected((prev) => ({ ...prev, [s.id]: e.target.checked }))}
/>
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
- also update comment and element type for omitting onChange Signed-off-by: Franz Heidl <franz.heidl@sap.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/ui-components/src/components/DataGridCheckboxCell/DataGridCheckboxCell.component.tsx:24
- The inline comment claims the bubbled
onChangewould be typed asChangeEventHandler<HTMLDivElement>, but forHTMLAttributes<HTMLDivElement>React typesonChangeas aFormEventHandler<HTMLDivElement>. Updating this avoids confusion for consumers reading the rationale for omittingonChangefrom the props.
// Omit onChange: React's synthetic onChange bubbles, so it would fire when the child Checkbox changes, but the type would be ChangeEventHandler<HTMLDivElement> — misleading consumers into thinking they're wiring up checkbox state. Wire onChange directly on the Checkbox child instead.
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Summary
This PR removes the long-WIPed
DataGridCheckboxCellcomponent and adds averticalAlignmentprop toDataGridCell, along with story improvements across the DataGrid family.During the initially planned streamlining of
DataGridCheckboxCell(removing its internalCheckboxrender in favour of a child-based approach), it became clear thatDataGridCellitself can cover the vertical centering thatDataGridCheckboxCellwas solely responsible for.With
DataGridCellnow able to override the parent grid's alignment per cell,DataGridCheckboxCellhas no remaining purpose and is removed entirely. Consumers who need a checkbox cell should use a regularDataGridCellwithverticalAlignment="center"and place aCheckboxas a child, wiring up state and handlers directly on it.Changes
DataGridCheckboxCell: removed entirelyDataGridCell: addverticalAlignmentprop overriding parentDataGridcellVerticalAlignmentper cellDataGridCellstories: addVerticalAlignmentOverridestory; add radio controls forverticalAlignmentDataGridCellstories: extendNoWrapstory with longer content and ellipsis truncation exampleDataGridCelltests: add missing tests forverticalAlignment,nowrap,colSpan, and prop passthroughDataGridHeaderFullyFeaturedstory: replaceDataGridCheckboxCellwithDataGridCellusingverticalAlignment="center"and a wiredCheckboxchildRelated Issues
Closes #1884
Screenshots (if applicable)
Testing Instructions
pnpm ipnpm run test DataGridCheckboxCell DataGridCellChecklist
PR Manifesto
Review the PR Manifesto for best practises.