-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[DataGridPremium] Make row grouping work with negative filtering #16004
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
base: master
Are you sure you want to change the base?
Conversation
Deploy preview: https://deploy-preview-16004--material-ui-x.netlify.app/ |
Hi @michelengelen, would you like to review this? |
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 your contribution @k-rajat19 ... I'll add some reviewers to this! 👍🏼
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.
Thank you!
This PR however seems to break the group column filtering in this demo: https://deploy-preview-16004--material-ui-x.netlify.app/x/react-data-grid/row-grouping/#single-grouping-column-2

Can you take a look?
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.
This might be a correct behavior 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.
IIUC, when mainGroupingCriteria
is invalid or not defined we check for leafField
, and if that is invalid or not defined we use the top-level grouping criteria, with that in mind I've also written getGroupingCriteria
method.
In this demo when we select default behaviour, it sets main grouping criteria to undefined and since there is no leafField it considers top level grouping criteria which is company
, so this looks correct behaviour to me.
const filteredRowsLookup: Record<GridRowId, boolean> = {}; | ||
const filteredChildrenCountLookup: Record<GridRowId, number> = {}; | ||
const filteredDescendantCountLookup: Record<GridRowId, number> = {}; | ||
const filterCache = {}; | ||
|
||
const getGroupingCriteria = () => { |
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 looks like we can call this function only once and use the result, instead of calling it in shouldFilterGroup
function for every node. WDYT?
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.
Also, what's the intetion of this function? I see that it returns null if the grouping column has leafField
, can you explain what's the purpose of that?
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.
shouldFilterGroup
method predicts where filtering can be applied based on the values infilterModel
and grouping criteria from getGroupingCriteria
method
we don't want to filter the group ( i.e, it returns false
) when any of the following conditions are true :
1- there is a valid leafField
2- there are quickFilterValues in filterModel
3- when filtering is applied on the column field which is ungrouped.
4- when rowGroupingColumnMode
is multiple
and when filtering can be applied on multiple grouping columns,
it returns false
when groupingField
property in group node from rowTree
doesn't match the last field in
groupFilterItems
.
5- when there is leaf
or footer
node in rowTree
.
I hope I've not missed any edge case to consider in shouldFilterGroup, also may be we can make things simple here but at the moment I'm not sure how.
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 looks like we can call this function only once and use the result, instead of calling it in
shouldFilterGroup
function for every node. WDYT?
yes, we can improve here
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.
This might be a correct behavior though 🤔
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has been inactive for 30 days. Please remove the stale label or leave a comment to keep it open. Otherwise, it will be closed in 15 days. |
Fixes #15605
Preview- https://deploy-preview-16004--material-ui-x.netlify.app/x/react-data-grid/row-grouping/