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

Allow adding children to node without refreshing the whole tree #417

Merged
merged 3 commits into from
May 10, 2023

Conversation

fsvergara
Copy link
Contributor

@fsvergara fsvergara commented Nov 30, 2022

Use case:
Dynamically add children (after an API call for example) to nodes without refreshing the whole tree (and expanding all nodes).

Problem:
When displaying a big tree, we'll fetch the children via API to avoid getting all information at once since it might be too big in size. The problem with that approach is that if we add those new nodes to the data prop, the whole tree will be refreshed with new ids and will have collapsed: false by default, so it will expand even the collapsed nodes, making the navigation confusing as you load more nodes.

Proposed solution:

  • Add dataKey prop to the Tree model. If the dataKey is not present or changes, we should update the whole tree, but if the data prop changes but the dataKey is the same, it means is the same tree, so we shouldn’t update.
  • Add addChildren method to the Node model exposed props, to allow adding children dynamically from each node.

Here's a screencast explaining the changes and the use case for this:

https://www.loom.com/share/70e372804b7d4d46a1b7a09564c5d3bf

@bkrem
Copy link
Owner

bkrem commented Jan 11, 2023

Apologies about the silence here, this clearly slipped through in my email notifications.

Thank you for this @fsvergara, especially the thoughtful and detailed PR description outlining why this is necessary based on your experience 🙏

Will try to review fully by the end of the week.

Btw, Github is showing me this but not clear what the conflict is:
Screenshot 2023-01-12 at 03 05 12

@dev-john-nguyen
Copy link
Contributor

This there any update on this MR? I would like to take advantage of these changes.

@fsvergara
Copy link
Contributor Author

Apologies about the silence here, this clearly slipped through in my email notifications.

Thank you for this @fsvergara, especially the thoughtful and detailed PR description outlining why this is necessary based on your experience 🙏

Will try to review fully by the end of the week.

Btw, Github is showing me this but not clear what the conflict is: Screenshot 2023-01-12 at 03 05 12

@bkrem sorry for the delay answering to this message. I just resolved the conflicts (I'm also importing RawNodeDatum to src/Node/index.tsx, that why there was a conflict)

@dev-john-nguyen
Copy link
Contributor

@bkrem Is it possible to get this reviewed and merged?

Copy link
Owner

@bkrem bkrem left a comment

Choose a reason for hiding this comment

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

Thanks again @fsvergara for the detailed PR, I didn't actually spot the loom video on the first pass, great context 💯

CI is failing on coverage threshold which I will adjust in this case. Would ideally have a test to cover this prop but since it's opt-in I think it's safe enough to go ahead without until I have the time to write one for it.

@bkrem bkrem merged commit 0f3d550 into bkrem:master May 10, 2023
@bkrem
Copy link
Owner

bkrem commented May 10, 2023

Published as https://github.com/bkrem/react-d3-tree/releases/tag/v3.6.0

h/t also to @dev-john-nguyen for helping drive this to completion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants