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

feat: useTreeData add replaceAll #7821

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rob-clayburn
Copy link
Contributor

@rob-clayburn rob-clayburn commented Feb 24, 2025

The immutable nature of the useTreeData's items means I couldn't find a way to re-initialise a list with new data returned from a subsequent API call.

This PR adds a new method to the list called replaceAll which will replace all items from the tree with those specified in the function call.

✅ Pull Request Checklist:

  • [ n/a ] Included link to corresponding React Spectrum GitHub Issue.
  • [ ✅ ] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • [ ✅ ] Updated documentation (if it already exists for this component).
  • [ n/a ] Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

The tests cover how it works, if you need to test in a component then I'm using it as :


// Data is the result of a fetch query to the backend which is re-run when a tree node is deleted, added etc.
export const MyTree = ({ data }) => {
  const list = useTreeData({
    initialItems: nodeTree,
  });

  useEffect(() => {
    list.replaceAll(nodeTree);
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [nodeTree]);

return <Tree items={list.items}>....</Tree>
};

@snowystinger
Copy link
Member

Thanks for the PR. We had a chance to discuss this as a team today. We have a few changes we'd like to propose.

We think the name could be confusing (and we may change our minds on this one a few times), right now the only proposed one is setRootItems. Would you mind updating to that?

We also want to do this and match the API in useListData.

Of note, this may be solved eventually in useAsyncTree. Or it could be solved today by implementing something like optimistic updates.

We are also a little wary because a function such as setRootItems wouldn't necessarily get rid of the selected items or any other state which might be tied to the tree data. This could result in unexpected behavior or keys being left around that no longer exist.

@rob-clayburn
Copy link
Contributor Author

thanks for replying,
I've updated setRootItems

We also want to do this and match the API in useListData.

Is that something you would like me to attempt to add in this PR?

@snowystinger
Copy link
Member

If you have the time, that would be super helpful. But I also understand if this is as far as you want to take it.

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.

2 participants