[internal] Headless virtualization plugin#21093
Conversation
cherniavskii
added
scope: data grid
|
Deploy preview: https://deploy-preview-21093--material-ui-x.netlify.app/ Bundle size report
|
4212fbf to
b5d0522
Compare
cherniavskii
changed the title
github-actions
Bot
added
the
PR: out-of-date
|
|
||
| const scrollbarVerticalRef = useEventCallback(this.refSetter('scrollbarVertical')); | ||
| const scrollbarHorizontalRef = useEventCallback(this.refSetter('scrollbarHorizontal')); | ||
| const scrollbarVerticalRef = useScrollbarRefCallback( |
There was a problem hiding this comment.
why was this not needed in the material grid?
There was a problem hiding this comment.
In the MUI X Data Grid it was contained in https://github.com/mui/mui-x/blob/master/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx
It didn't matter much, because both virtualization layout and virtual scrollbar are internal details in MUI X Data Grid.
But in the headless data grid, we want as easy setup as possible, hence the change.
It's worth noting that these changes do not impact MUI X Data Grid, since it uses LayoutDataGridLegacy, while in the headless data grid we use LayoutDataGrid.
There was a problem hiding this comment.
That means we have duplicated code now, right?
That would be nice to avoid. That being said I'll probably refactor the virtual scrollbars code (for other products using the virtualizer - some might want them) and I'd handle the duplication then, so feel free to leave it in (maybe with a comment?) for now.
There was a problem hiding this comment.
Oh, I forgot that LayoutDataGridLegacy extends LayoutDataGrid, I'll fix that
this is now handled in the virtualizer layout
arminmeh
left a comment
arminmeh
left a comment
There was a problem hiding this comment.
Some minor things to consider, but we can merge the PR and deal with those later as well
| /> | ||
| )} | ||
| {hasScrollY && ( | ||
| <DataGridVirtualScrollbar position="vertical" hasOppositeScrollbar={hasScrollX} /> |
There was a problem hiding this comment.
Not so important, since it is a demo app, but
With macOS's option to automatically show scrollbars based on mouse or trackpad, the scrollbars never show.
| } | ||
| }); | ||
|
|
||
| resolvedUserPlugins.forEach((plugin) => { |
There was a problem hiding this comment.
I don't have any use case in mind, but at some point we might want to allow user plugins to be even before the internal plugins...
This is just a thought, you can leave the code as-is for now.
| }); | ||
| }); | ||
|
|
||
| describe('rows to render', () => { |
There was a problem hiding this comment.
a test where virtualization strips some rows is missing
| }); | ||
| }); | ||
|
|
||
| describe('columns to render', () => { |
There was a problem hiding this comment.
a test where virtualization strips some columns is missing
|
|
||
| // Verify the hook exists and can be used | ||
| const dimensions = apiRef.current!.api.virtualization.hooks.useDimensions; | ||
| expect(dimensions).to.be.a('function'); |
There was a problem hiding this comment.
Should we test the number of rows being rendered (number increasing by decreasing the height)?
| ); | ||
|
|
||
| // Verify the API is available | ||
| expect(apiRef.current!.api.virtualization.getScrollPosition).to.be.a('function'); |
There was a problem hiding this comment.
Can we check the actual numbers here?
| }); | ||
| }); | ||
|
|
||
| describe('hooks', () => { |
There was a problem hiding this comment.
Not sure how much value it makes to test that we are getting a function. Most of these are covered by type checks
| const DEFAULT_ROW_HEIGHT = 52; | ||
| const DEFAULT_COLUMN_WIDTH = 100; |
There was a problem hiding this comment.
Should these two values be in the constants of the respective plugins?
| const rowBufferPx = params.rowBufferPx ?? 150; | ||
| const columnBufferPx = params.columnBufferPx ?? 150; |
There was a problem hiding this comment.
Can you extract the default buffer as a constant?
| previousRun.processorNames.every((name, index) => name === processorNames[index]); | ||
|
|
||
| let startIndex = 0; | ||
| let input = this.options.getInitialValue(); |
There was a problem hiding this comment.
To save some computation time, we can set this to null here and call getInitialValue() if it does not get filled by the previous output.
4 participants
No description provided.