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.

Sign up for GitHub

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

Jump to bottom

[TreeView] Rename nodeId to itemId #12418

Merged
merged 13 commits into from
Mar 14, 2024
Merged

[TreeView] Rename nodeId to itemId #12418

merged 13 commits into from
Mar 14, 2024

Conversation

noraleonte
Copy link
Contributor

@noraleonte noraleonte commented Mar 11, 2024 •
edited

part of #12262

Follow-up:

  • UseTreeViewFocusParameters['onNodeFocus'] => onItemFocus (DONE)
  • clean up internals

Changelog

Breaking changes

The required nodeId prop used by the TreeItem has been renamed to itemId for consistency:

 <TreeView>
-    <TreeItem label='Item 1' nodeId='one'>
+    <TreeItem label='Item 1' itemId='one'>
 </TreeView>

@noraleonte noraleonte added breaking change component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Mar 11, 2024
@noraleonte noraleonte added this to the 7.0.0 milestone Mar 11, 2024
@noraleonte noraleonte self-assigned this Mar 11, 2024
@mui-bot
Copy link

mui-bot commented Mar 11, 2024 •
edited

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

Updated pages:

Generated by 🚫 dangerJS against 33c7c81

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 11, 2024
@github-actions GitHub Actions
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 Mar 11, 2024
@noraleonte noraleonte marked this pull request as ready for review March 11, 2024 17:38
flaviendelangle
flaviendelangle reviewed Mar 12, 2024
View reviewed changes
docs/data/migration/migration-tree-view-v6/migration-tree-view-v6.md Outdated
@@ -387,3 +387,14 @@ This will help create a new headless version of the `TreeItem` component based o
)
}
```

### ✅ Rename `nodeId` to `itemId`
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth mentioning the nodeId prop received by the ContentComponent prop of TreeItem as well (it's in this PR from what I saw)

Copy link
Member

Choose a reason for hiding this comment

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

WDYT, would it make sense to mention this section before any other section using itemId (first is on line 59) to reduce confusion? 🤔

@github-actions GitHub Actions
Copy link

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 Mar 12, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 12, 2024
@github-actions GitHub Actions
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 Mar 12, 2024
LukasTy
LukasTy approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy 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, LGTM. 👏 💯
Leaving a few nitpick comments. 😉

docs/data/migration/migration-tree-view-v6/migration-tree-view-v6.md Outdated
@@ -387,3 +387,14 @@ This will help create a new headless version of the `TreeItem` component based o
)
}
```

### ✅ Rename `nodeId` to `itemId`
Copy link
Member

Choose a reason for hiding this comment

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

WDYT, would it make sense to mention this section before any other section using itemId (first is on line 59) to reduce confusion? 🤔

docs/data/migration/migration-tree-view-v6/migration-tree-view-v6.md Outdated Show resolved Hide resolved
packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.ts Outdated Show resolved Hide resolved
packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.types.ts Show resolved Hide resolved
noraleonte and others added 2 commits March 14, 2024 15:39
@noraleonte @LukasTy
Apply suggestions from code review - Lukas
725e334 
Co-authored-by: Lukas <llukas.tyla@gmail.com>
Signed-off-by: Nora <72460825+noraleonte@users.noreply.github.com>
@noraleonte
update guide
33c7c81
flaviendelangle
flaviendelangle approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Looks great!

@noraleonte noraleonte merged commit a518d0a into mui:next Mar 14, 2024
17 checks passed
@noraleonte noraleonte deleted the rename-nodeId-to-itemId branch March 14, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaviendelangle flaviendelangle flaviendelangle approved these changes

@LukasTy LukasTy LukasTy approved these changes

Assignees

@noraleonte noraleonte

Labels
breaking change component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@noraleonte @mui-bot @flaviendelangle @LukasTy