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

[TreeView] Automatic parents and children selection #14899

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 9, 2024

No rush to review this PR since it's not for v7, breaking change preparations on the pickers are more pressing 🙏

Apply #13757 for the Tree View
Closes #12883
Doc preview

Thanks to @MBilalShafi for all the work on the Data Grid feature, porting it to the Tree View is a piece of cake now 🙏

There is a super small BC on the legacy TreeItem, I propose to keep the feature for the alpha, it will be the main new feature for the Tree View in the stablefor people without big datasets.

  • Build feature
  • Add doc
  • Add tests
  • Move the checkbox logic from useTreeItem2 to useTreeViewSelection.itemPlugin (it's becoming quite large with the indeterminate support)
  • Support indeterminate checkbox state (same behavior as the grid)

@flaviendelangle flaviendelangle self-assigned this Oct 9, 2024
@flaviendelangle flaviendelangle added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Oct 9, 2024
@mui-bot
Copy link

mui-bot commented Oct 9, 2024

} from './useTreeViewSelection.types';
import { UseTreeViewItemsSignature } from '../useTreeViewItems';

function getCheckboxStatus({
Copy link
Member Author

Choose a reason for hiding this comment

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

This would benefit from a selector when available

@@ -75,25 +75,36 @@ Use the `onItemSelectionToggle` prop if you want to react to an item selection c

{{"demo": "TrackItemSelectionToggle.js"}}

## Parent / children selection relationship
## Automatic parents and children selection
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically a copy paste of the grid doc
If we modify this one, I'd be in favor of having Sam check the changes and apply the same to the grid one.

*
* @default { parents: false, descendants: false }
*/
selectionPropagation?: TreeViewSelectionPropagation;
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as the doc, it's a copy paste of the grid JSDoc

@flaviendelangle flaviendelangle marked this pull request as ready for review October 10, 2024 11:44
@flaviendelangle flaviendelangle force-pushed the propagate-selection branch 3 times, most recently from 48702ae to 6889bc1 Compare October 10, 2024 13:39
Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Amazing addition 🎉 That was super fast 🤭

@Kavan72
Copy link

Kavan72 commented Oct 15, 2024

@flaviendelangle Thank you for the amazing work. I have one question: If the parent checkbox is in an indeterminate state and the user checks it, shouldn’t the first click select all the children, and the second click uncheck all of them along with the parent?

@flaviendelangle
Copy link
Member Author

Hey @Kavan72, thanks for the feedback

The action when clicking on an indeterminate parent was the source of a lot of debate over on the Data Grid.
The current behavior their is to unselect all by default but have it configurable using the indeterminateCheckboxAction prop which has the following type:

  /**
   * If `select`, a group header checkbox in indeterminate state (like "Select All" checkbox)
   * will select all the rows under it.
   * If `deselect`, it will deselect all the rows under it.
   * Works only if `checkboxSelection` is enabled.
   * @default "deselect"
   */
indeterminateCheckboxAction?: 'select' | 'deselect'

On the Tree View, I followed the default behavior of the grid.
We can introduce the same indeterminateCheckboxAction prop in a follow up, it should be almost trivial to do it 👍

@Kavan72
Copy link

Kavan72 commented Oct 15, 2024

Hey @Kavan72, thanks for the feedback

The action when clicking on an indeterminate parent was the source of a lot of debate over on the Data Grid. The current behavior their is to unselect all by default but have it configurable using the indeterminateCheckboxAction prop which has the following type:

  /**
   * If `select`, a group header checkbox in indeterminate state (like "Select All" checkbox)
   * will select all the rows under it.
   * If `deselect`, it will deselect all the rows under it.
   * Works only if `checkboxSelection` is enabled.
   * @default "deselect"
   */
indeterminateCheckboxAction?: 'select' | 'deselect'

On the Tree View, I followed the default behavior of the grid. We can introduce the same indeterminateCheckboxAction prop in a follow up, it should be almost trivial to do it 👍

That would be great feature 🙂

@flaviendelangle
Copy link
Member Author

Issue created: #14979

Given the low complexity of the topic, I suspect that we should be able to ship that a few weeks after the main PR (depending on the bandwidth we have)

@MBilalShafi
Copy link
Member

MBilalShafi commented Oct 15, 2024

For v8, on the Data Grid side, it is also one of the considerations to make select the default behavior and remove indeterminateCheckboxAction prop, mainly aiming simplicity. I'd love to decide on it together with the Tree View for consistency.

Context: 1, 2

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 16, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! v8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TreeView] Support parent / children selection relationship
5 participants