-
-
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
[DataGridPremium] Use valueGetter
to get row group keys
#16016
[DataGridPremium] Use valueGetter
to get row group keys
#16016
Conversation
@@ -1605,7 +1605,7 @@ describe('<DataGridPremium /> - Row grouping', () => { | |||
expect(getColumnValues(1)).to.deep.equal(['', '0', '3', '', '1', '4', '', '2']); | |||
}); | |||
|
|||
it('should not use valueGetter to group the rows when defined', () => { | |||
it('should use valueGetter to group the rows when defined', () => { |
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 turned out that the current behavior was intentional.
I disagree with this, but maybe I don't see the full picture.
I know it's been 3 years 😅, but still – @flaviendelangle if you remember any additional details about this decision – please let me know 🙂
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.
We discussed it some time ago with @michelengelen and the problem is that I don't remember the reason why we did that 😆
But I remember we did it on purpose (maybe a bad one though).
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.
@cherniavskii just a small update to say that, given I don't remember the rationals behind this current behavior AND we have a clear recurring pain point with it, I think it make sense to move forward with your PR.
The likelyhood that my rational was really important and yet that I don't remember it is pretty low
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.
Thanks for the update, I already forgot about this PR!
Deploy preview: https://deploy-preview-16016--material-ui-x.netlify.app/ |
packages/x-data-grid-premium/src/hooks/features/rowGrouping/gridRowGroupingUtils.ts
Show resolved
Hide resolved
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
f7bee11
to
5898f54
Compare
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.
LGTM
export const getRowValue = ( | ||
row: GridValidRowModel, | ||
colDef: GridColDef, | ||
apiRef: React.MutableRefObject<GridApiCommunity>, |
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.
apiRef: React.MutableRefObject<GridApiCommunity>, | |
apiRef: RefObject<GridApiCommunity>, |
Fixes #16015