mui-x
mui-x copied to clipboard
[TreeView] Add support for checkbox selection
Fixes #6019 Doc preview
The main inspiration is https://mui.com/x/react-data-grid/row-selection/#checkbox-selection
- [x] Add checkbox selection on
TreeItem
- [x] Add checkbox selection on
TreeItem2
- [x] Add doc
- [x] Add tests
Deploy preview: https://deploy-preview-11452--material-ui-x.netlify.app/
Updated pages:
- docs/data/tree-view/rich-tree-view/selection/selection.md
- docs/data/tree-view/simple-tree-view/selection/selection.md
Generated by :no_entry_sign: dangerJS against 1aa1a585ee1ceab03d33088c4e448aff4c27a902
This pull request has conflicts, please resolve those before we can evaluate the pull request.
I agree, and I suppose users can style the default checkbox, which probably cover the majority of customization use cases, so maybe we can release the slot on a follow up?
Right now they can't pass props to the default checkbox specifically on the tree view (they'd need a lot to do it) All they can do it use the theme to pass default props to all their checkbox throughout their application (even the one not in tree view).
Their is nothing preventing us from adding the slot in a follow up though. I think it will quickly be useful, but there is no reason to be blocking.
I suppose this is also a separate PR. Must it be delivered first?
For now this is only used internally since nobody except us can build plugins, so nothing blocking here :+1:
This pull request has conflicts, please resolve those before we can evaluate the pull request.
Hi,
I gave this a try to see if we could add it to our app but ran into a few issues.
- The multiselect checkbox are unusable on mobile as the control button can't be clicked. Should clicking the checkbox toggle just that selection rather than overriding the selection to get that to be more user friendly?
- Selecting a parent doesn't automatically select all the children, and vice versa. Is this something that the MUI implementation is considering?
I understand that this is just a base implementation, just curious on the plans with this in the future.
Thanks for taking the time to test it!
The multiselect checkbox are unusable on mobile as the control button can't be clicked. Should clicking the checkbox toggle just that selection rather than overriding the selection to get that to be more user friendly?
Do we agree that we have the exact same problem on mobile with the current selection (where you click on the item to select)? People can't use multi select on mobile as fair as I can see. I can have a look at ARIA's spec to see what they recommend.
Selecting a parent doesn't automatically select all the children, and vice versa. Is this something that the MUI implementation is considering?
Same than above, this logic should probably be shared with and without checkboxes. The DataGrid has the same kind of topics (see #4248), it will most-likely be possible to implement this logic in user-land. The main challenge for doing a built-in implementation is how we handle data updates. Imagine you have an item with 3 children, you select it, it selects the 3 children. Then you have some data call that adds a 4th children, should it be selected? So for me here we need to discuss with the DataGrid team to have a consistent selection experience.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
Hi,
I gave this a try to see if we could add it to our app but ran into a few issues.
- The multiselect checkbox are unusable on mobile as the control button can't be clicked. Should clicking the checkbox toggle just that selection rather than overriding the selection to get that to be more user friendly?
- Selecting a parent doesn't automatically select all the children, and vice versa. Is this something that the MUI implementation is considering?
I understand that this is just a base implementation, just curious on the plans with this in the future.
Both of these points are how I would expect a tree select with checkboxes to work. In fact, it's how other tree selects work. One example -- https://primereact.org/treeselect/#check
This pull request has conflicts, please resolve those before we can evaluate the pull request.
Imagine you have an item with 3 children, you select it, it selects the 3 children. Then you have some data call that adds a 4th children, should it be selected?
I would suggest that the 4th child should be left unselected, and the parent moved to an indeterminate state, but I'm interested to hear of any use cases that this wouldn't work for.
The existing multi-selection seems broken on desktop – selected items aren't highlighted. This might be the same issue that @LukasTy was referring to :
we firstly need to at least avoid the styling regression on mobile
I'm not if sure check-boxes without multi-selection makes sense as a UI paradigm, since at the point it's effectively a radio button that looks like a checkbox (albeit one with "select-none"), so if we support it, perhaps we should advise against it as a pattern?
Similarly, having to hold a modifier key to multi-select checkboxes breaks expectations for how checkboxes work.
Keyboard navigation seems a bit broken, depending on whether you use cursor keys to select the tree view items, or tab to select a checkbox (at which point multi-selection isn't possible).
Holding the shift key and moving the focus up or down toggles the state of all items except the one that was originally focused, but I'm on the fence about whether that's an issue, or expected. It isn't how Finder on Mac works at least...
I missed one. The demos overflow the demo container:
I missed one. The demos overflow the demo container:
The Tree Item height was increased a lot when checkboxes were enabled. I don't think it's something we want, I removed the padding of the checkbox to avoid it and we not longer have the overflow.
I'm not if sure check-boxes without multi-selection makes sense as a UI paradigm, since at the point it's effectively a radio button that looks like a checkbox (albeit one with "select-none"), so if we support it, perhaps we should advise against it as a pattern?
We have several solutions here:
- We ignore the
multiSelect
prop and always enablemultiSelect
when using checkbox selection (not super consistent IMHO) - We change the default value of
multiSelect
to betrue
when using checkbox selection (little more complex to understand but the default behavior is coherent) - We keep things as is (the default behavior is not super logical, and not coherent with the grid)
Similarly, having to hold a modifier key to multi-select checkboxes breaks expectations for how checkboxes work.
True, I reverted this change to behave like the data grid Clicking on multiple checkbox will select all of them Clicking on a checkbox with shift will expand the range
Keyboard navigation seems a bit broken, depending on whether you use cursor keys to select the tree view items, or tab to select a checkbox (at which point multi-selection isn't possible).
I add tabIndex={-1}
to the checkbox for now to make it consistent.
Looks like on the grid you want move from checkbox to checkbox using tab.
The only diff is that on the grid, when a row is selected, pressing tab from outside of the grid will focus the checkbox instead of the row.
If we want this behavior, I can replicate the hacky code of the grid (which adds tabIndex={-1}
imperatively on the row.
Concerning the automatic selection of children. The grid has no such thing: https://stackblitz.com/edit/react-r8ltu2?file=Demo.tsx,package.json
If we want to implement what @mbrookes is describing, then the selection would only be determined by the selection state of the leaves. And for me the main challenge is how we represent this parent-child selection relationship in the state.
Imagine we have the following tree:
- 1
- 1.1
- 1.2
- 2
If we select 1.1
, then its parent should be visually in an indeterminate state. How does it impact the selection state?
Does it equal ['1.1']
?
If we select 1.1
and 1.2
, then their parent should be visually selected. How does it impact the selection state?
Does it equal ['1', '1.1', '1.2']
or ['1.1', '1.2']
And what happens if the user controls the selection and passes an invalid value (like ['1', '1.1']
), or a value that only contains the parent (like ['1']
), do we call onSelectedItemsChange
with the full model? (['1', '1.1', '1.2']
) in this case)?
We mainly need to be consistent here and do not monkey-patch the behavior. For example, only store the leaves on the model and deduce the parent selection state from their descendants.
And if this behavior is not strictly limited to checkbox selection (but also impacts the classic selection), then also take this aspect into account.
I think the conclusion on the grid was that it was too much on an headache and it was simpler to give the tools to the user to manually select the children when a parent is selected (which is far from ideal of course).
EDIT: I propose to not support the children automatic selection on this 1st version to avoid delaying the release too much. And to create an issue to explore how to support it: https://github.com/mui/mui-x/issues/12883.
The support could look something like:
- Add a new prop
enableAutomaticDescendantSelection
(name to be determined of course) - If this prop is enabled, then the model only contains the selection of the leaves. Any id in the model that is not the leave is ignore. Items with children have their selection deduced from the selection state of their children. When using the checkbox selection, we support the indeterminate state.
WDYT @LukasTy @noraleonte ?
EDIT: I propose to not support the children automatic selection on this 1st version to avoid delaying the release too much. And to create an issue to explore how to support it.
The support could look something like:
- Add a new prop
enableAutomaticDescendantSelection
(name to be determined of course)- If this prop is enabled, then the model only contains the selection of the leaves. Any id in the model that is not the leave is ignore. Items with children have their selection deduced from the selection state of their children. When using the checkbox selection, we support the indeterminate state.
I have no objections against separating the delivery of the complete feature behavior.
The proposed approach makes sense, but I feel that the enableAutomaticDescendantSelection
might make more sense as the default behavior.
WDYT? 🤔
I am also in favor of creating a follow up for automatic children selection 👌 But I agree that the behavior should exist and should probably be the default 🤔
I don't really like the idea of the selection of the parents being determined strictly by the state of their children. It could become very messy for nested trees. A flow I think also makes sense is the one that Figma (and other similar software) uses for layer selection.
If you have a tree like this:
- 1
- 1.1
- 1.2
- 2
- 2.1
- 2.2
If you select 2.1 & 2.2 the selection state is ['2.1','2.2']. However, if you select 1, it automatically selects the children as well, and the state is ['1', '1.1', '1.2'].
https://github.com/mui/mui-x/assets/72460825/3b5c834d-214c-4cea-b795-55788372ae8d
but I feel that the enableAutomaticDescendantSelection might make more sense as the default behavior.
It's a BC so it could only be opt-out in v8 even if the feature is released before (which is far from obvious)
But I don't have a strong opinion about what the default behavior should be in the end.
If we feel like none of those two use-cases is used a lot more than the other, then maybe it would be a better DX to have an enum prop rather than a boolean (something like descendantSelection?: 'independant' | 'automatic'
with perhaps automatic
as the default.
@noraleonte , interesting This is yet another behavior, which don't seems to make sense with checkbox but do make sense without them. Selecting a parent selects the children, but selecting all the children do not select the parent (if I understand correctly).
With this one, I do agree that we don't have a problem with the model structure, it can always represent what has been selected and the visual selection is basically isSelected(itemId) || hasSomeAncestorSelected(itemId)
.