Skip to content

fix(RAC): Tree DnD followup + docs #8302

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

Merged
merged 50 commits into from
Jun 4, 2025
Merged

fix(RAC): Tree DnD followup + docs #8302

merged 50 commits into from
Jun 4, 2025

Conversation

reidbarber
Copy link
Member

@reidbarber reidbarber commented May 23, 2025

Story fixes:

  • fixed drop indicator not showing outside of iframe
  • fixed child items not moving when dragged to second tree
  • enabled dragging in second tree

DnD fixes:

  • enabled switching between ambiguous drop targets (e.g. 'after last child' or 'after parent') when mouse dragging, based on drag X and Y movement.
  • fixed drop target not scrolling into view

Docs:

  • added docs that match existing collection DnD docs

✅ Pull Request Checklist:

  • 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).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented May 27, 2025

Build successful! 🎉

@reidbarber reidbarber changed the title fix(RAC): Tree DnD followup fix(RAC): Tree DnD followup + docs May 27, 2025
@rspbot
Copy link

rspbot commented May 27, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 27, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 29, 2025

Build successful! 🎉

snowystinger
snowystinger previously approved these changes May 29, 2025
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

One issue I found, if I try to move an item by mouse from its current position, to be the last item in the previous sibilings's children, I can't do that.
Steps:
with mouse, open Projects
try to move Reports to be after Project 5

Overall looks pretty good though, will give approval in case any of this would be followup

@LFDanLu LFDanLu added the release label Jun 3, 2025
…ey is selected (#8337)

* Fix tree docs example by accounting for child keys that are included in a selected parent

* update getKeys to automatically filter out child keys if parent key is also selected

* fix infinite loop and add test
@rspbot
Copy link

rspbot commented Jun 3, 2025

Build successful! 🎉

Comment on lines +97 to +99
if (potentialTargets.length === 0) {
return {type: 'root'};
}
Copy link
Member

Choose a reason for hiding this comment

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

After some experimenting, I decided returning root here makes the most sense. You can test this via http://localhost:9003/?path=/story/react-aria-components--tree-with-drag-and-drop&args=dropFunction:onReorder;shouldAcceptItemDrop:folders;shouldAllowInsert:!false&providerSwitcher-express=false&strict=true (replace localhost with whatever build link you are using) and then:

  1. Drag Projects into the right tree
  2. Expand the newly dropped folder
  3. Attempt to drop more items from the left tree onto the right tree and notice the root being returned as the target when hovering the whitespace or a non-folder item

Returning the root when hovering an invalid drop target seems to make the most sense here and is inline with the behavior see if you ONLY have onRootDrop available (see https://reactspectrum.blob.core.windows.net/reactspectrum/2aa18518f267f6fafd9225d8bc867db2d2c9a747/storybook/index.html?path=/story/tableview-drag-and-drop--drag-between-tables-root-only&providerSwitcher-express=false).

Perhaps it is weird that the end user doesn't have a way to distinguish between an invalid drop target and if the collection only takes root drops, but it seems better than potentially blocking someone from a root drop completely if there wasn't any white space to drop on.

Copy link
Member

Choose a reason for hiding this comment

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

also, note that the above table story will exhibit the same behavior as the tree even if it had other allowed drop operations (e.g. onItemDrop), the root is targeted if hovering over a invalid drop target

@rspbot
Copy link

rspbot commented Jun 3, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jun 3, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jun 3, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jun 4, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jun 4, 2025

Build successful! 🎉

let node = collection.getItem(key);
let isChild = false;
if (node && node?.parentKey) {
for (let potentialParentKey of selectionManager.selectedKeys) {
Copy link
Member

Choose a reason for hiding this comment

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

Could be faster to just walk up the parent keys and check if any of them are also in selectedKeys. Currently this is an n^2 algorithm since you have a nested loop over all keys, but it could be done in linear time since selectedKeys is a set. You just need to check if any of the parent keys of a node are also in selectedKeys.

outer: for (let key of selectionManager.selectedKeys) {
  let node = collection.getItem(key);
  while (node && node.parentKey != null) {
    node = collection.getItem(node.parentKey);
    if (node && selectionManager.isSelected(node.key) {
      continue outer;
    }
  }

  keys.add(key);
}

@rspbot
Copy link

rspbot commented Jun 4, 2025

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jun 4, 2025

## API Changes

react-aria-components

/react-aria-components:TreeRenderProps

 TreeRenderProps {
+  allowsDragging: boolean
   isEmpty: boolean
   isFocusVisible: boolean
   isFocused: boolean
+  selectionMode: SelectionMode
   state: TreeState<unknown>
 }

@reidbarber reidbarber enabled auto-merge June 4, 2025 18:36
@reidbarber reidbarber added this pull request to the merge queue Jun 4, 2025
Merged via the queue into main with commit 473c611 Jun 4, 2025
30 checks passed
@reidbarber reidbarber deleted the tree-dnd-followup branch June 4, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants