mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.53k stars 1.31k forks source link

[TreeView] `canMoveItemToNewPosition` params not providing correct `itemId` to function #14175

Closed charlieprime24 closed 3 months ago

charlieprime24 commented 3 months ago

Steps to reproduce

Steps:

  1. Have a page with a RichTreeViewPro component and a tree with multiple items
  2. Configure item reordering according to docs
  3. Add an implementation of canMoveItemToNewPosition prop
  4. Drag and drop a tree item
  5. See that params.itemId passed to canMoveItemToNewPosition is the ID of the item at the new position and not the ID of the item being dropped

Current behavior

Happy to be told i'm wrong on this but it seems the ID of the item at the new position is passed into canMoveItemToNewPosition (params.itemId) instead of the ID of the item being dropped.

Expected behavior

The ID of the item being dropped is passed into canMoveItemToNewPosition (params.itemId).

Context

Trying to implement a drag and drop tree view for displaying a users and groups org structure. Because of this there is no way to know which item is being dropped in canMoveItemToNewPosition.

Your environment

npx @mui/envinfo ``` System: OS: macOS 14.5 Binaries: Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node npm: 10.2.4 - ~/.nvm/versions/node/v20.11.0/bin/npm pnpm: Not Found Browsers: Chrome: 127.0.6533.100 Edge: Not Found Safari: 17.5 npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.10 @mui/core-downloads-tracker: 5.14.4 @mui/icons-material: ^5.14.1 => 5.14.3 @mui/lab: ^5.0.0-alpha.137 => 5.0.0-alpha.139 @mui/material: ^5.14.1 => 5.14.4 @mui/material-nextjs: ^5.15.5 => 5.15.5 @mui/private-theming: 5.16.6 @mui/styled-engine: 5.16.6 @mui/styles: ^5.14.1 => 5.14.4 @mui/system: 5.16.7 @mui/types: 7.2.15 @mui/utils: 5.16.6 @mui/x-data-grid: 6.11.0 @mui/x-data-grid-pro: ^6.10.0 => 6.11.0 @mui/x-date-pickers: 6.11.0 @mui/x-date-pickers-pro: ^6.10.0 => 6.11.0 @mui/x-internals: 7.12.0 @mui/x-license: 7.12.0 @mui/x-license-pro: ^6.10.0 => 6.10.2 @mui/x-tree-view: ^7.6.2 => 7.12.1 @mui/x-tree-view-pro: ^7.12.1 => 7.12.1 @types/react: 18.2.14 => 18.2.14 react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 typescript: ~5.1.3 => 5.1.6 ```

Search keywords: tree-view pro Order ID: 90357

michelengelen commented 3 months ago

To help us diagnose the issue efficiently, could you provide a stripped-down reproduction test case using the latest version? A live example would be fantastic! ✨

For your convenience, our documentation offers templates and guides on creating targeted examples: Support - Bug reproduction

Just a friendly reminder: clean, functional code with minimal dependencies is most helpful. Complex code can make it tougher to pinpoint the exact issue. Sometimes, simply going through the process of creating a minimal reproduction can even clarify the problem itself!

Thanks for your understanding! 🙇🏼

charlieprime24 commented 3 months ago

Reprod: https://stackblitz.com/edit/react-hckveb?file=Demo.tsx

michelengelen commented 3 months ago

Thanks @charlieprime24 ... I think you are correct. The description on the API docs is not very distinctive as well: params.itemId: The id of the item to check. I would say the itemId should be the one moving, right @flaviendelangle? As the new position is being passed in (including the id of the new parent) as well

flaviendelangle commented 3 months ago

Thanks for the detailed feedback :pray: Two things here from what I can see

  1. Yes it should be the moving item that is passed to canMoveItemToNewPosition and it seems that we instead pass the target which is a clear bug
  2. Yes the JSDoc could be a lot more explicit

I'll open a PR to fix both topics :+1:

charlieprime24 commented 3 months ago

Awesome thanks guys!

github-actions[bot] commented 3 months ago

:warning: This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@charlieprime24: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

charlieprime24 commented 3 months ago

Hey guys, when will this get released?

flaviendelangle commented 2 months ago

Tomorrow or on Friday, we have a weekly release :+1: