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] Remove instance.getTreeItemIdAttribute #14667

Merged
merged 9 commits into from
Sep 26, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 19, 2024

Extracted from #14210

This is typically the type of instance methods that require us to re-render every item since it's used in the render of the item.
We need to make sure the id attribute of every item is up to date at all time, but with this PR we only have to listen to treeId in order to update the id attribute of the item.

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

mui-bot commented Sep 19, 2024

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

Generated by 🚫 dangerJS against 0bffd84

@flaviendelangle flaviendelangle marked this pull request as ready for review September 19, 2024 08:14
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.

Great work! 🚀

@@ -23,8 +10,19 @@ export interface UseTreeViewIdParameters {

export type UseTreeViewIdDefaultizedParameters = UseTreeViewIdParameters;

export interface UseTreeViewIdState {
id: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question: any reason why we are nesting treeId? Is it to be able to identify the state related to a certain plugin more easily? Should we do the same for other plugin states?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it to be able to identify the state related to a certain plugin more easily?

Yes, I took the habit to namespace everything in the state

Should we do the same for other plugin states?

Do we have some plugins where it is not the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

UseTreeViewFocusState and UseTreeViewLabelState do not follow this convention from what I see

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I fixed those in #14210 👼
But that's something I should definitely extract

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants