Skip to content

ListView: Draggable Rows #2593

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 83 commits into from
Feb 28, 2022
Merged

ListView: Draggable Rows #2593

merged 83 commits into from
Feb 28, 2022

Conversation

reidbarber
Copy link
Member

@reidbarber reidbarber commented Nov 24, 2021

Closes #2495

Notes:

  • Includes a DragHandle icon because the one in @spectrum-icons/workflow is outdated. We can use that once it is updated.
  • custom render preview is not supported yet

Known issues:

  • clicking to drag an item will toggle selection
  • clicking on the action menu button doesn't open the menu
  • It is a bit weird that some items select on mouse down and some on mouse up. I think if dnd is enabled for the list, we should make all items select on mouse up, not just for items that are draggable.
  • Mouse down on drag indicator shows a focus ring. Sometimes it is the default browser one, and other times it is ours. We should get rid of the default browser one, and also find out why ours sometimes shows.
  • Focus ring on drag indicator doesn't hide after pressing enter to initiate a drag. Makes it look like it's still focused.
  • Note to people testing: performing a drop via screenreader doesn't quite work (at least in TalkBack double tapping doesn't actually drop the items), seems to be something non-specific to ListView so out of scope for this PR.
  • Additionally it seems like the drop operation on mobile doesn't quite recognize multiple item drops the same way like it does in desktop, something to do with writeToDataTransfer from what I can tell. onDrop will have the proper number of items in onDrop's items when performed on desktop but only have one item when performed on mobile (and seemingly combines all the items together instead)? (ANDROID specific?)
  • weirdness with focus when using enter to begin dragging and after dropping: see ListView: Draggable Rows #2593 (comment)

✅ 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:

Test draggable stories at ListView -> Draggable Rows:

  • Dragging should not work if the drag operation starts on an intractable element within row.
  • Row c, e, and m are non-draggable. Row f is disabled and is thus non-draggable
  • Test mouse dragging
  • Test keyboard dragging (tab to drag handle)

🧢 Your Project:

RSP

@adobe-bot
Copy link

Build successful! 🎉

@reidbarber reidbarber changed the title ListView: Draggable Rows [WIP] ListView: Draggable Rows Nov 24, 2021
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

we only import types from these packages so put them as dev deps
@adobe-bot
Copy link

Build successful! 🎉

@@ -143,7 +143,8 @@ export interface DraggableCollectionProps {
onDragStart?: (e: DraggableCollectionStartEvent) => void,
onDragMove?: (e: DraggableCollectionMoveEvent) => void,
onDragEnd?: (e: DraggableCollectionEndEvent) => void,
getItems: (keys: Set<Key>) => DragItem[],
getItems?: (keys: Set<Key>) => DragItem[],
Copy link
Member

@devongovett devongovett Feb 28, 2022

Choose a reason for hiding this comment

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

nit: this should not be optional

Suggested change
getItems?: (keys: Set<Key>) => DragItem[],
getItems: (keys: Set<Key>) => DragItem[],

devongovett
devongovett previously approved these changes Feb 28, 2022
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Other than one small nit above, looks good!

@adobe-bot
Copy link

Build successful! 🎉

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.

I'm happy to discuss and dismiss. Does the order of events seem a little strange? (Read bottom up like a console output)

dragEnd
onDrop
onDropExit
onDropActivate
onDropEnter
dragStart

I think I'd have expected

dragEnd
onDropExit
onDrop
onDropActivate
onDropEnter
dragStart

Though I may just not know enough about dnd native event orders.

Why can't I drag and drop two items into the textfield in https://reactspectrum.blob.core.windows.net/reactspectrum/99a6ff0791b2b98ae068feec9892fe871aaf14c3/storybook/index.html?path=/story/listview--draggable-rows I have to delete the contents before I can do it again with another item and sometimes it shows a focus ring when I drag something over it?

I can get stuck in drag and drop mode pretty easily if I try to use the mouse, should using the mouse cancel the dnd mode? I also can't get to the textfield using the keyboard to drop it on that.

Focus ring on safari is a bit janky
Screen Shot 2022-02-28 at 12 36 32 PM

Otherwise, working with mouse/keyboard on Chrome and Safari

@devongovett devongovett merged commit 3dc96e5 into main Feb 28, 2022
@devongovett devongovett deleted the ListView-DnD branch February 28, 2022 20:30
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.

ListView drag & drop integration
6 participants