-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Fix isRowSelectable
when paginationMode='server'
#3647
[DataGrid] Fix isRowSelectable
when paginationMode='server'
#3647
Conversation
These are the results for the performance tests:
|
isRowSelectable
when paginationMode='server'
if (selectedRowsLookup[id] == null) { | ||
isSelected = false; | ||
} else if (typeof rootProps.isRowSelectable === 'function') { | ||
isSelected = rootProps.isRowSelectable(apiRef.current.getRowParams(id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this change is good. In the previous way, the selection lookup was already filtered all at once and memoized. Now isRowSelectable
might be called several times for the same row. I'm OK to call it for every currentPage.rows
and memoize the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought behind this is that renderedRows
will never be hundreds of rows long whereas currentPage.rows
might have 100 000 items.
But I could memoize renderedRows
and create a memoized selection lookup based on renderedRows
.
That way we don't loop on the potentially very long currentPage.rows
and we only re-run when the 1st and/or last rendered rows changes.
packages/grid/_modules_/grid/components/columnSelection/GridHeaderCheckbox.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx
Outdated
Show resolved
Hide resolved
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Fixes #3614
Small performance improvement: we don't call
isRowSelectable
for every row inGridBody
but only on rendered rows.For the header checkbox, the behavior remain incoherent with
paginationMode="server"
because we don't know if there are unselected rows on other pages.I think we can fix that one in a follow up PR by adding a
getSelectAllStatus
callback for instance, which would return'all' | 'none' | 'some'
.