-
Notifications
You must be signed in to change notification settings - Fork 40
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
Bug with node deletion when controlling node expansion #133
Comments
@mihna123 I’m not used to seeing setData called in a recursive method. I’m concerned that that is the reason why you’re seeing this. So can you rewrite your sandbox to utilize pure functions that calculate the new tree data and then call setData once and only once? But maybe I’m reading your code incorrectly. So while you consider my suggestion, @kpustakhod can you speak with our PM about getting time to investigate this in a future sprint? I don’t think sprint 14.1 or 14.2 have room, so let’s ask for 14.3. @mihna123 for clarity that means that we won’t be able to consider your PR till at least 4 weeks from now. |
@callwait the reason I didn’t merge the PR is because I think this is caused by an error in the consumer code. See above. |
I'm facing the same issue when manually changing the tree nodes and expanding all nodes. Reproduction :
Does the PR #132 fix this issue? |
@Ke1sy, @yhy-1, @kpustakhod please mention this to Kevin and Jason so it can be worked. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions. |
For what it's worth, I was able to work around this bug by creating a hash of all my data IDs and abusing the |
Hi! Could you show your example of solving this problem? |
Basically this:
|
Thanks a lot! It works to me! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions. |
Used the hacky workaround from @lb-pendula after spending waaaay too much time trying to figure out why a filtered tree was causing errors. |
@sjakos @ivan75238 @mihna123 @lb-pendula I’m sorry that it’s taken so long to get this bug looked at. I’m requesting our management to consider our volunteerism budget to use on this bug. It’s hard to rationalize spending budget on this when it’s a bug that we aren’t experiencing at our company. But we’ll try. Reopening the issue now. |
@mihna123 Calling When I made that small change (to only call it once), the issue you reported went away. https://codesandbox.io/p/sandbox/thirsty-black-2mmgjp (In this fork, you'll have to manually expand nodes after you add them) |
@dgreene1 Hey, just wanted to say that work would be appreciated! This project fills a real niche that exists for apps that (for legacy) reasons are stuck on React 16, and can't use the TreeView in newer versions of MUI. |
@mellis481 I did the same thing without calling The issue happens if the user wants to have all nodes expanded by default, so your code is not that relevant i think. |
@mellis481 I cant access your codesandbox, did you set it to public? |
@mihna123 Try now. |
@mellis481 Sandbox works now, thank you. So if I expand all nodes with |
I just updated my sandbox to use a state variable to maintain expanded IDs and it works fine. I also tested the code you shared along with some console.logs and it looks like the component is re-rendering a bunch of times such that when it goes to delete the node, it has already been removed. So I think the way you've implemented the component in the codesandbox is the cause of the problem. |
@mellis481 Works better with a state variable. I am getting error now when trying to add a new node after deleting all nested nodes. See below for an example: |
@mihna123 I don't have time at the moment to provide a full working codesandbox. I will say that, after spending a little bit of time trying to change tree data, it's not super easy to do it having to manage data in the state of the component that instantiates a tree. Please take a look at this example, though, as it it's a different approach to dynamically changing tree data that may be helpful. https://dgreene1.github.io/react-accessible-treeview/docs/examples-MultiSelectCheckboxAsyncControlled |
@mellis481 Very interesting! I posted this issue a while back when working on a project I no longer work on, so I no longer have a personal interest in this. I will try to see if there is a valid PR I can provide since I have some time on my hands, but if you guys don't see how fixing this would benefit the project I'm fine with this issue being closed. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions. |
Describe the bug
This bug happens when the user wants to have all branches expanded and chooses to have a dynamic node adding/deleting system.
By adding two or more nested branches and then deleting them an error will occur : "Node with id=x doesn't exist in the tree"
As an example i have created a sandbox. Clicking the button "add node" will add a node as a child to a node that has been selected, clicking the button "delete" will delete node that has been selected (and all it's children).
To Reproduce
Steps to reproduce the behavior:
Expected behavior
It is expected that you get an error "Node with id=x doesn't exist in the tree"
Screenshot
This is the look of the tree at step number 4:
Desktop (please complete the following information):
The text was updated successfully, but these errors were encountered: