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] Do not use outdated version of the state to compute new label first char in RichTreeView #12512

Merged

Conversation

flaviendelangle
Copy link
Member

Fixes #12510

@flaviendelangle flaviendelangle self-assigned this Mar 20, 2024
@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Mar 20, 2024
@flaviendelangle flaviendelangle changed the title [TreeView] Do not used outdated version of the state to compute new label first char in RichTreeView [TreeView] Do not used outdated version of the state to compute new label first char in RichTreeView Mar 20, 2024
@mui-bot
Copy link

mui-bot commented Mar 20, 2024

Deploy preview: https://deploy-preview-12512--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 35fbec8

);
if (itemElement) {
itemElement.blur();
if (node) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not directly related but I noticed this bug while writing the tests.
If we remove the currently focused item from props.items, then its TreeItem while call removeFocusedItem, but the state does not contain the item anymore so it crashes.

@flaviendelangle flaviendelangle changed the title [TreeView] Do not used outdated version of the state to compute new label first char in RichTreeView [TreeView] Do not use outdated version of the state to compute new label first char in RichTreeView Mar 20, 2024
};

params.items.forEach(processItem);
Object.values(state.nodes.nodeMap).forEach(processItem);
Copy link
Member Author

Choose a reason for hiding this comment

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

Btw this will support stuff like apiRef.current.updateItem when we add this kind of API, the previous approach did not.

It's still far from perfect because we loop through the whole dataset, but until we actually have partial dataset update it's good enough.

@flaviendelangle flaviendelangle marked this pull request as ready for review March 20, 2024 13:02
@flaviendelangle flaviendelangle changed the base branch from next to master March 21, 2024 14:10
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.

Looks good to me! 🎉
Left a tiny comment about naming 😬

@flaviendelangle flaviendelangle merged commit 6968393 into mui:master Mar 25, 2024
17 checks passed
@flaviendelangle flaviendelangle deleted the outdated-item-node-firstChar branch March 25, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TreeView] RichTreeView throws error when using dynamic items
3 participants