Skip to content
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] Automatic parents and children selection #13757

Merged
merged 51 commits into from
Oct 4, 2024

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Jul 8, 2024

https://deploy-preview-13757--material-ui-x.netlify.app/x/react-data-grid/row-grouping/#automatic-parents-and-children-selection

Resolves #4248
Resolves #6177

Follow up

Stuff that will follow after the PR merge:

@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request feature: Selection Related to the data grid Selection feature feature: Tree data Related to the data grid Tree data feature feature: Row grouping Related to the data grid Row grouping feature labels Jul 8, 2024
@mui-bot
Copy link

mui-bot commented Jul 8, 2024

Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature feels great! Congrats, @MBilalShafi!

I noticed it currently only works consistently with one level of hierarchy:

Screen.Recording.2024-07-10.at.11.51.39.mov

Which drives me to a second point, should devs have a way to fine-tune the propagation behavior? (e.g. how many levels should it select, should it also select all parents in the hierarchy)
What do you think, and how could that look like?

docs/data/data-grid/row-grouping/row-grouping.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 10, 2024

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 11, 2024
Co-authored-by: José Rodolfo Freitas <[email protected]>
Signed-off-by: Bilal Shafi <[email protected]>
@MBilalShafi MBilalShafi changed the title [DataGridPro] Add option to propagate row selection for nested rows [DataGridPro] Automatic parents and children selection Jul 11, 2024
@MBilalShafi MBilalShafi changed the title [DataGridPro] Automatic parents and children selection [DataGridPremium] Automatic parents and children selection Jul 11, 2024
@MBilalShafi MBilalShafi marked this pull request as ready for review July 16, 2024 09:12
@MBilalShafi MBilalShafi requested a review from a team July 16, 2024 09:13
Comment on lines +91 to +95
const checkboxPropsSelector = getCheckboxPropsSelector(
id,
rootProps.rowSelectionPropagation?.parents ?? false,
);
const { isIndeterminate, isChecked } = useGridSelector(apiRef, checkboxPropsSelector);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of using a selector here if it's recreated every time?

Copy link
Member Author

@MBilalShafi MBilalShafi Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selector is created every time yes, but due to the underlying ref used in useGridSelector it will be initialized only once and provide proper reactivity for the isIndeterminate and isChecked props.

The need to recreate the selector should also be solved when we shift to selectors with arguments (in v8) as mentioned in #13757 (comment)

Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so createSelector is only used to avoid direct state access (reusing selectors we already have), but we don't benefit from its memoization (because we recreate the selector on every render).

How about using the v8 selector here?

Copy link
Member Author

@MBilalShafi MBilalShafi Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using the v8 selector here?

Yes, that will be the end goal when we enter the v8 pre-release phase. Currently, it's not possible as the parameters' order of v7 selectors and v8 selectors is different.
If we use the v8 selector here, we have to convert the reused selectors to v8 ones for them to work properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, you're right. The old selectors aren't compatible with v8 selectors 👍🏻

Copy link
Contributor

@romgrk romgrk Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selectors are never reactively bound. There just wasn't any case where it made sense. For it to make sense we'd need a use-case like const x = useGridSelector(api, condition ? selectorA : selectorB), but I don't see any case where the type of x would make sense. And even if there was, selectors are so cheap that it's easier to select both slices and do the condition outside. It would have also prevented useGridSelector(api, state => state.x.y).

Comment on lines +91 to +95
const checkboxPropsSelector = getCheckboxPropsSelector(
id,
rootProps.rowSelectionPropagation?.parents ?? false,
);
const { isIndeterminate, isChecked } = useGridSelector(apiRef, checkboxPropsSelector);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so createSelector is only used to avoid direct state access (reusing selectors we already have), but we don't benefit from its memoization (because we recreate the selector on every render).

How about using the v8 selector here?

Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @MBilalShafi!

docs/data/data-grid/row-grouping/row-grouping.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-grouping/row-grouping.md Outdated Show resolved Hide resolved
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

docs/data/data-grid/row-grouping/row-grouping.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-grouping/row-grouping.md Outdated Show resolved Hide resolved
MBilalShafi and others added 2 commits October 3, 2024 22:52
Co-authored-by: Andrew Cherniavskii <[email protected]>
Co-authored-by: José Rodolfo Freitas <[email protected]>
Signed-off-by: Bilal Shafi <[email protected]>
Signed-off-by: Bilal Shafi <[email protected]>
@MBilalShafi MBilalShafi merged commit 978a438 into mui:master Oct 4, 2024
18 checks passed
@MBilalShafi MBilalShafi deleted the propagate-row-selection branch October 4, 2024 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Row grouping Related to the data grid Row grouping feature feature: Selection Related to the data grid Selection feature feature: Tree data Related to the data grid Tree data feature new feature New feature or request
Projects
None yet
10 participants