-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] Extract getRowId
API method as a selector
#16487
Conversation
@@ -112,18 +113,11 @@ export const useGridRows = ( | |||
[apiRef], | |||
); | |||
|
|||
const getRowIdProp = props.getRowId; | |||
const getRowId = React.useCallback<GridRowApi['getRowId']>( |
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.
Any thoughts about deprecating it in favor of the added utility?
Deprecated in favor of the new selector gridRowIdSelector
We could remove all internal instances later.
Deploy preview: https://deploy-preview-16487--material-ui-x.netlify.app/ |
packages/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid-premium/src/hooks/features/aggregation/createAggregationLookup.ts
Outdated
Show resolved
Hide resolved
getRowId
API method as static functiongetRowId
API method as a selector
This comment was marked as outdated.
This comment was marked as outdated.
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.
Are we sure about deprecating these? 🤔
Discovering and calling a method is much easier than discovering the selector and importing it.
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.
Discovering and calling a method is much easier than discovering the selector and importing it.
It's a good point.
Here are the arguments I have in mind in favor of the deprecation:
- Both
getRowId
andgetRowNode
are meant to be used internally more than 90% of the times and having API methods for them kind-of feels redundant. - Less API surface (especially for the cases where there's an easy workaround) is always better for maintainability etc.
- Using selectors is not something that we rarely use now, after the reactivity improvements, it's been used very frequently at multiple places, I'd personally try to use documentation to elaborate the use of those selectors to solve the discoverability, if we feel a knowledge gap.
Since we are deprecating them and not removing them until next major, if there's a discoverability pain reported by a noticeable number of users we can always bring them back. (We could avoid deprecating them in v7) Wdyt?
This comment was marked as outdated.
This comment was marked as outdated.
Cherry-pick PRs will be created targeting branches: v7.x |
Fixes #16103
Fixes #16413
Better fix for #14691
Reuses the props state logic initially committed in #12343
Changelog
getRowId
API method.gridRowIdSelector
should be used instead.getRowNode
API method.gridRowNodeSelector
should be used instead.