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] Remove use of row.id when id prop is available #1371
Conversation
packages/grid/_modules_/grid/hooks/features/filter/gridFilterSelector.ts
Outdated
Show resolved
Hide resolved
return sortedIds.reduce((res, id) => { | ||
res[id] = idRowsLookup[id]; | ||
return res; | ||
}, {} as Record<GridRowId, GridRowModel>); |
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.
same here. not sure why you refactored the return type
Not entirely sure how |
8db1a46
to
20b8522
Compare
@@ -60,6 +60,7 @@ export const GridViewport: ViewportType = React.forwardRef<HTMLDivElement, {}>( | |||
<GridRowCells | |||
columns={visibleColumns} | |||
row={r} |
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.
could we cleanup here please? row?
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.
@@ -60,6 +60,7 @@ export const GridViewport: ViewportType = React.forwardRef<HTMLDivElement, {}>( | |||
<GridRowCells | |||
columns={visibleColumns} | |||
row={r} | |||
id={r.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.
Would that one change with the next PR?
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.
Yes, the next PR changes to get the id from the Map.
@@ -20,10 +20,15 @@ import { useLogger } from '../../utils/useLogger'; | |||
import { useGridState } from '../core/useGridState'; | |||
import { getInitialGridRowState, InternalGridRowsState } from './gridRowsState'; | |||
|
|||
// TODO remove after all row.id are removed | |||
export function addGridRowId(rowData: GridRowData, getRowId?: GridRowIdGetter): GridRowModel { |
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.
can that one go now?
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.
It's gone in #1377.
20b8522
to
bfc0fe3
Compare
Related to #1119, #1200, #1315
This is the first batch of changes to satisfy the need of removing
id
from the rowData. In this PR I'm replacingrow.id
with theid
prop in all places where it's already available. It has no breaking changes. In the next PR I'll do the rest of the work that requires changes in the API too.