Add theming property to define max checkbox size #1061
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for a new checkboxMaxSize theme setting so consumers can control the maximum dimensions of all checkboxes in the grid. Key changes include:
- Propagate
theme.checkboxMaxSizeinto the drawing routines and default parameters. - Extend the
Themeinterface, CSS variables, API docs, and storybook examples to expose the new property. - Update mock utilities and render calls to use the configurable size.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| draw-checkbox.ts | Use theme.checkboxMaxSize fallback in drawCheckbox |
| data-grid-render.header.ts | Pass theme.checkboxMaxSize into header checkbox draws |
| theme-support.stories.tsx | Add checkboxMaxSize to example dark and custom themes |
| 08-theming.stories.tsx | Document new checkboxMaxSize (and roundingRadius) in the table |
| utils.tsx (data-editor stories) | Remove assertNever at end of lossyCopyData |
| styles.ts | Inject --gdg-checkbox-max-size and add checkboxMaxSize to Theme |
| marker-cell.tsx | Use theme.checkboxMaxSize for row marker sizing |
| boolean-cell.tsx | Use theme.checkboxMaxSize for boolean cell sizing |
| API.md | Add checkboxMaxSize (and roundingRadius) to the theme reference |
Comments suppressed due to low confidence (3)
packages/core/src/data-editor/stories/utils.tsx:107
- The removal of
assertNever(target)at the end oflossyCopyDatameans the function may no longer enforce exhaustiveness and could implicitly returnundefined. Reintroduce an assertion or explicitthrowin the default case to ensure allEditableGridCellkinds are handled.
}
packages/core/src/internal/data-grid/render/draw-checkbox.ts:24
- We should add unit tests to verify the fallback logic for
maxSize, especially when consumingtheme.checkboxMaxSizeversus the default value.
let checkBoxWidth = getSquareWidth(maxSize ?? theme.checkboxMaxSize ?? 32, height, theme.cellVerticalPadding);
packages/core/src/common/styles.ts:52
- Add a snapshot or style test to ensure the
--gdg-checkbox-max-sizeCSS variable is correctly emitted whencheckboxMaxSizeis set or omitted.
...(theme.checkboxMaxSize === undefined ? {} : { "--gdg-checkbox-max-size": `${theme.checkboxMaxSize}px` }),
| const rectBordRadius = style === "circle" ? 10_000 : theme.roundingRadius ?? 4; | ||
| let checkBoxWidth = getSquareWidth(maxSize, height, theme.cellVerticalPadding); | ||
| const rectBordRadius = style === "circle" ? 10_000 : (theme.roundingRadius ?? 4); | ||
| let checkBoxWidth = getSquareWidth(maxSize ?? theme.checkboxMaxSize ?? 32, height, theme.cellVerticalPadding); |
There was a problem hiding this comment.
Is the a reason why its 32 here instead of 18?
There was a problem hiding this comment.
Just because it was the default value for maxSize parameter in this specific function (see the function signature above). Not sure why it's 32, but to keep everything as is probably good to keep using it here.
There was a problem hiding this comment.
I would check drawCheckbox doesn't look visually different with a defined checkboxMaxSize value / try to figure out if we can just remove this magic 32 too.
There was a problem hiding this comment.
ok, sounds good. Changed it to have checkboxMaxSize always defined as 18 and removed 32. Did some manual checking, and everything looks good.
Add an optional
checkboxMaxSizethat allows for the configuration of the max size (height & width) of checkboxes. This is applied to all checkboxes, e.g, boolean cell & row markers.