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] Improve the expansion API #11476

Merged
merged 10 commits into from
Jan 2, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 20, 2023

Closes #10689

Changelog

Breaking changes

  • The expansion props have been renamed to better describe their behaviors:

    Old name New name
    onNodeToggle onExpandedNodesChange
    expanded expandedNodes
    defaultExpanded defaultExpandedNodes
      <TreeView
    -   onNodeToggle={handleExpansionChange}
    +   onExpandedNodesChange={handleExpansionChange}
    
    -   expanded={expandedNodes}
    +   expandedNodes={expandedNodes}
    
    -   defaultExpanded={defaultExpandedNodes}
    +   defaultExpandedNodes={defaultExpandedNodes}
      />

@flaviendelangle flaviendelangle self-assigned this Dec 20, 2023
@flaviendelangle flaviendelangle added breaking change component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Dec 20, 2023
@mui-bot
Copy link

mui-bot commented Dec 20, 2023

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

Generated by 🚫 dangerJS against 5b3088a

@flaviendelangle
Copy link
Member Author

I'll add the tests and codemod once I have a confirmation that we agree to go with this new API.
We discussed it a few weeks ago in a meeting.

And I'd also like to confirm if I should apply the same renames to the selection API (defaultSelected => defaultSelectNodes, selected => selectedNodes, onNodeSelect => onSelectedNodesChange + create a new onNodeSelectionToggle).
For me it makes a lot of sense to at least do the renaming. For the new onNodeSelectionToggle we can wait for users request but I'm pretty sure it's something we want to add so maybe it's better to just stay consistent with the expansion and add it now.

Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

Looking good ... you just missed a change to the jsdocs. Other than that I only have 2 small comments on the wording. Very nice!

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.

Nice improvement! 🎉 The renaming makes sense to me 👍
For consistency's sake I would rename the selection api as well 🤔

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 21, 2023
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 Dec 21, 2023
Copy link

github-actions bot commented Jan 2, 2024

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

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

github-actions bot commented Jan 2, 2024

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 Jan 2, 2024
@flaviendelangle
Copy link
Member Author

@michelengelen your requested change is applied 👍

Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

nice ... thanks! LGTM! :shipit:

@flaviendelangle flaviendelangle merged commit a71f448 into mui:next Jan 2, 2024
17 checks passed
@flaviendelangle flaviendelangle deleted the expanded-nodes branch January 2, 2024 13:09
@LukasTy LukasTy changed the title [tree view] Improve the expansion API [TreeView] Improve the expansion API Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 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] Provide the id of the newly expanded / collapsed node in onNodeToggle
4 participants