Make loading cell able to render a loading skeleton #832
Conversation
| import { GridCellKind, type LoadingCell } from "../internal/data-grid/data-grid-types.js"; | ||
| import type { InternalCellRenderer } from "./cell-types.js"; | ||
|
|
||
| function getRandomNumber(x: number, y: number): number { |
There was a problem hiding this comment.
Would add a function comment saying it maps two number pseudo-randomly to the range [-1, 1] since that's not immediately clear.
| @@ -313,6 +313,8 @@ export interface BaseGridCell { | |||
| /** @category Cells */ | |||
| export interface LoadingCell extends BaseGridCell { | |||
| readonly kind: GridCellKind.Loading; | |||
| readonly skeletonWidth?: number; | |||
| readonly skeletonVariability?: number; | |||
There was a problem hiding this comment.
Think instead of skeletonVariability, skeletonWidth should either be a static number or a dynamic (col: number, row: number) => number, so it can left up to developer to be pseudo-random or deterministic.
There was a problem hiding this comment.
The callback seems not needed as you get that information inside getCellContent. The variability is just there to make it easier to achieve semi-organic skeletons without writing your own randomizer. I just didn't want it to be the obvious fallback that people use Math.random() which is rather slow.
| } | ||
|
|
||
| const hpad = theme.cellHorizontalPadding; | ||
| const rectHeight = 18; |
There was a problem hiding this comment.
Is the 18 here based on the current defaults for line height & font size? In that case, could it be calculated from whatever is defined in the theme?
There was a problem hiding this comment.
There wasn't a good option I could think of really. Using the cell height - 2*cellVerticalPadding is probably the most consistent method, but it is likely to look bad for some people. The only other things I could think to do were to make it configurable as a theme param, make it configurable as a cell param, or to make it parse the font param (which is a really bad idea).
There was a problem hiding this comment.
I've attempting to strike a better balance in the recent commit
No description provided.