-
-
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
[TreeView] Add selectItem
and getItemDOMElement
methods to the public API
#13485
[TreeView] Add selectItem
and getItemDOMElement
methods to the public API
#13485
Conversation
@@ -56,9 +56,10 @@ Use the `setItemExpansion` API method to change the expansion of an item. | |||
apiRef.current.setItemExpansion( | |||
// The DOM event that triggered the change | |||
event, | |||
// The ID of the item to expand or collapse | |||
// The id of the item to expand or collapse |
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.
I tried to unify a bit the wording for those boolean parameters throughout the Tree View codebase
And to always have the same descriptions for the params in the JSDoc and in the doc section
Deploy preview: https://deploy-preview-13485--material-ui-x.netlify.app/ Updated pages: |
@flaviendelangle This DX looks perfectly agreeable to me 👍🏻 |
newSelected = [itemId].concat(cleanSelectedItems); | ||
} else { | ||
newSelected = cleanSelectedItems; | ||
} | ||
} else { | ||
// eslint-disable-next-line no-lonely-if | ||
if (newValue === false) { | ||
if (isSelected === false || (isSelected == null && instance.isItemSelected(itemId))) { |
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.
Small fix: in single selection, if isSelected
is not defined and the item is selected we should de-select.
I fixed the usage of this method in useTreeItemState
and useTreeItem2Utils
to keep the same behavior on the UI.
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.
Very nice addition 👌 The DX is good, and the docs look nice. Thanks for adding this 🎉 Left a little nitpick, but looks good to me otherwise
packages/x-tree-view/src/internals/plugins/useTreeViewSelection/useTreeViewSelection.types.ts
Outdated
Show resolved
Hide resolved
How does
I suggest to add a new method to Currently the docs are missing an explicit section for The above would best fit in such section and also here: Nothing is mentioning the Imperative API collides when the Tree is "controlled". |
If you provide
Could you expand on this? I don't understand your use case. |
I had no idea that I assumed, logically, that TreeView will never change a controlled state "under the hood", since that state could have been delicately cherry-picked, and I am uncertain how could the TreeView guess how to consolidate existing Since that was my assumption, I thought developers should consolidate the A simplified example for const apiRef = useTreeViewApiRef();
const [expanded, setExpanded] = useState(...initial array of calculated ids...);
useEffect(() => {
const expandedPath = apiRef.current?.getItemExpansionPath(selectedId);
expandedPath && setExpanded(s => s.concat(expandedPath)) // here I could do whatever
}, [apiRef, setExpanded, selectedId]);
|
Well, if you call
We don't change the controlled state "under the hood", we call the callback that lets you update the controlled state. If you control But note that the goal of those imperative methods is to allow you to update imperatively the expanded items without having to control the model to do it. For your code snippet, if I understand correctly your use case, you are trying to expand a deeply nested item while making sure all it's parent are also expanded in the process (to always have the selected item always visible).
apiRef.current.selectItem({
event,
itemId,
shouldExpandAncestors: true,
});
apiRef.current.setItemExpansion({
event,
itemId,
isExpanded: true,
shouldExpandAncestors: true,
}); I think it's better to have this kind of behavior built in inside the component because it's way easier to implement for the end user and the use case is far from being niche. |
what if I want to use
For that I wrote a utility method which traverses the tree upwards from a certain given I have a complex tree where I need need to programmatically expand and collapse multiple branches in the tree, to visually show only the portions of the tree which are relevant to certain different scenarios in my app.
Therefore for me working with a controlled For example, I've added a prop for my Tree wrapper for Maybe there should be a docs page with some recipes examples, like how to programmatically expand the whole tree. right now there is no easy one-method way to expand everything without the knowledge of all the ids of the nodes. Here's a video showcasing how the tree is used in my SaaS app (mocked simplified data):tree-demo.mp4The tree is a tool to filter data in a large table component (
There's a ton of customization going on here, which might be beneficial for others. I assume many who are using this tree component must spend their time writing the exact same utility methods again and again. It might be beneficial to expose some common use cases to save people's time. |
Closes #10113
selectItem doc preview
getItemDOMElement doc preview
@bharatkashyap I propose to expose a method that allow you to easily retrieve the DOM element rather than a method that allow you to scroll.
I first tried to implement something like
apiRef.current.scrollToItem(itemId)
, but I was basically copy-pasting the entire API of thescrollIntoView
method.Your use case can be achieved as follow:
And if you are using multi selection you might prefer:
I'll add some tests once we agree on the DX