Update simplified DnD handler api and memoize useDnDHooks #3533

LFDanLu · 2022-09-19T20:39:43Z

Followup review items from #3266

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

RSP

LFDanLu · 2022-09-19T20:45:20Z

packages/@react-aria/dnd/src/useDroppableCollection.ts

-    let {draggingCollectionRef, draggingKeys} = globalDndState;
-    let isInternalDrop = draggingCollectionRef?.current != null && draggingCollectionRef?.current === ref?.current;
+    let {draggingKeys} = globalDndState;
+    let isInternal = isInternalDropOperation(ref);


A bunch of these isInternal checks use ref instead of relying on the global state's dropCollectionRef because I felt it was safer this way (global state is spooky in general). Since these are all within useDroppableCollection, the ref should be equal to dropCollectionRef for the most part (some exceptions exist where we want to calculate isInternal before we even set dropCollectionRef)

packages/@react-aria/dnd/src/utils.ts

adobe-bot · 2022-09-19T20:47:32Z

Build successful! 🎉

isInternalDrop -> isInternal. Use target provided to onDrop for the util handlers

adobe-bot · 2022-09-20T21:36:06Z

Build successful! 🎉

…r Android slight optimization in DragManager by removing extraneous setDropCollectionRef in favor of drag end handling isInternal logic instead. Also fix detection of Android virtual clicks, element clicked via click() will have pointerType="" and detail=0 so we can rely on the previous detail=0 logic for that case instead

directory may be a type that is common among user data so it may accidentally conflict with our special directory keyword in acceptedDragTypes. To resolve this we change it so acceptedDragTypes allows a Symbol equivalent that we defined for directory

LFDanLu · 2022-09-20T23:22:30Z

packages/@react-aria/dnd/package.json

@@ -26,7 +26,6 @@
    "@react-aria/utils": "^3.13.3",
    "@react-aria/visually-hidden": "^3.4.1",
    "@react-stately/dnd": "3.0.0-alpha.10",


Was doing some auditing and have a clarifying question: should things like @react-stately/dnd/@react-types/* be in devDeps instead if they are only used to import types/interfaces? I was under the impression that we could do a import type for things like that and thus avoid needing to include them within the dependencies because they only matter during dev (aka typechecking)

LFDanLu · 2022-09-20T23:24:38Z

packages/@react-aria/interactions/src/utils.ts

@@ -33,7 +33,7 @@ export function isVirtualClick(event: MouseEvent | PointerEvent): boolean {
  // Android TalkBack's detail value varies depending on the event listener providing the event so we have specific logic here instead
  // If pointerType is defined, event is from a click listener. For events from mousedown listener, detail === 0 is a sufficient check
  // to detect TalkBack virtual clicks.
-  if (isAndroid() && (event as PointerEvent).pointerType != null) {
+  if (isAndroid() && (event as PointerEvent).pointerType) {


click events triggered by .click() will have a pointerType = "" and detail === 0 so we can avoid this check and fallback to the detail === 0 check below

LFDanLu · 2022-09-20T23:25:08Z

packages/@react-spectrum/dnd/src/useDnDHooks.ts

@@ -68,45 +68,46 @@ export interface DnDOptions extends Omit<DraggableCollectionProps, 'preview' | '
 }

 export function useDnDHooks(options: DnDOptions): DnDHooks {
-  let {


This diff is really just me moving everything into a single useMemo

packages/@react-types/shared/src/dnd.d.ts

devongovett · 2022-09-21T01:27:59Z

packages/@react-aria/dnd/src/utils.ts

@@ -159,12 +161,12 @@ export class DragTypes implements IDragTypes {
    this.includesUnknownTypes = !hasFiles && dataTransfer.types.includes('Files');
  }

-  has(type: string) {
-    if (this.includesUnknownTypes || (type === 'directory' && this.types.has(GENERIC_TYPE))) {
+  has(type: string | DirectorySymbolType) {


does string | symbol work here? Might be good to keep this a little more vague rather than only allowing directorySymbol, in case we add more special symbol types in the future.

devongovett · 2022-09-21T01:29:23Z

packages/@react-aria/dnd/src/utils.ts

@@ -18,6 +18,8 @@ import {Key, RefObject} from 'react';
 import {useId} from '@react-aria/utils';

 const droppableCollectionIds = new WeakMap<DroppableCollectionState, string>();
+export const directorySymbol = Symbol();


We haven't really done something like this before, but do we want to use upper case for constants? I think that's a fairly common convention. Something like DIRECTORY_DRAG_TYPE? If not, I still think naming it with drag type in the name rather than "symbol" makes sense.

devongovett · 2022-09-21T01:42:46Z

packages/@react-spectrum/dnd/src/useDnDHooks.ts

+      ...(isDraggable ? dragHooks : {}),
+      ...(isDroppable ? dropHooks : {}),
+      ...(isDraggable || isDroppable ? {isVirtualDragging} : {})
+    };


Instead of creating the objects unconditionally and then merging them, you could assign properties from within an if statement? Maybe a little more efficient.

let hooks = {}; if (isDraggable) { hooks.useDraggableCollectionState = /* ... */ } // etc.

also expand acceptedDragTypes to accept any symbol in case we add more unique drag types in the future

also skips calling the util handlers if filteredItems is empty

this allows the user to define what drop operations a droppable collection accepts, making it so copy or link can become the new default operation.

devongovett · 2022-09-21T23:45:38Z

packages/@react-aria/dnd/src/useDroppableCollection.ts

@@ -70,9 +70,9 @@ export function useDroppableCollection(props: DroppableCollectionOptions, state:
    let filteredItems = items;
    if (acceptedDragTypes) {


I guess this check isn't needed anymore? Or maybe it should be if (acceptedDragTypes !== 'all' || shouldAcceptItemDrop) as an optimization?

ah yep, thanks for catching this

…eact-spectrum into dnd_api_simplification_followups

adobe-bot · 2022-09-22T00:08:02Z

Build successful! 🎉

this avoids the need for adding a dev dep of react-spectrum/dnd to the types packages

LFDanLu · 2022-09-22T00:34:19Z

packages/@react-types/list/src/index.d.ts

@@ -21,7 +21,6 @@ import {
  SpectrumSelectionProps,


I would love to delete the @react-types/list package now that aria/spectrum define and export the types but a bit worried about if people are using it since it have been released.

would it be weird to change this package to re-export the types from aria/spectrum instead? only for backward compatibility I guess. not sure how important tho

feels a bit weird to have the dependencies go the other way, but at least it wouldn't be circular since nothing else should depend on this types package. I'll go ahead and update

adobe-bot · 2022-09-22T00:37:42Z

Build successful! 🎉

for backcompat. We define and export these props from aria/spectrum packages now, the react-types/list packages is no longer necessary since it is only used for one component

adobe-bot · 2022-09-22T17:33:52Z

Build successful! 🎉

adobe-bot · 2022-09-22T19:58:32Z

Build successful! 🎉

LFDanLu changed the title ~~Update simplified DnD handler api and memoize useDnDHooks return~~ (WIP) Update simplified DnD handler api and memoize useDnDHooks return Sep 19, 2022

LFDanLu changed the title ~~(WIP) Update simplified DnD handler api and memoize useDnDHooks return~~ (WIP) Update simplified DnD handler api and memoize useDnDHooks Sep 19, 2022

LFDanLu commented Sep 19, 2022

View reviewed changes

packages/@react-aria/dnd/src/utils.ts Show resolved Hide resolved

Base automatically changed from dnd_api_simplification to main September 20, 2022 17:18

LFDanLu added 4 commits September 20, 2022 14:28

updating dnd util handler api

15bd874

isInternalDrop -> isInternal. Use target provided to onDrop for the util handlers

adding utility function for checking if it is a internal drop operation

9e96fae

memoize everything in useDnDHooks

f831552

remove extra itemProcessor

60993aa

LFDanLu force-pushed the dnd_api_simplification_followups branch from d69cdbc to 60993aa Compare September 20, 2022 21:29

LFDanLu added 3 commits September 20, 2022 15:00

making directory drag type a Symbol

3bde69a

directory may be a type that is common among user data so it may accidentally conflict with our special directory keyword in acceptedDragTypes. To resolve this we change it so acceptedDragTypes allows a Symbol equivalent that we defined for directory

auditing package.jsons for dnd stuff

d282671

LFDanLu changed the title ~~(WIP) Update simplified DnD handler api and memoize useDnDHooks~~ Update simplified DnD handler api and memoize useDnDHooks Sep 20, 2022

LFDanLu commented Sep 20, 2022

View reviewed changes

packages/@react-types/shared/src/dnd.d.ts Outdated Show resolved Hide resolved

LFDanLu marked this pull request as ready for review September 20, 2022 23:32

devongovett reviewed Sep 21, 2022

View reviewed changes

LFDanLu added 6 commits September 21, 2022 12:03

replacing directorySymbol with DIRECTORY_DRAG_TYPE

5f803cc

also expand acceptedDragTypes to accept any symbol in case we add more unique drag types in the future

adjusting dnd hook object creation

9e0e4dc

changing default for acceptedDragTypes back to all

5df8143

also skips calling the util handlers if filteredItems is empty

use getDropOperation to determine final drop operation if provided

b5f21c0

this allows the user to define what drop operations a droppable collection accepts, making it so copy or link can become the new default operation.

Merge branch 'main' into dnd_api_simplification_followups

0f23e7c

re-export DIRECTORY_DRAG_TYPE from react-spectrum/dnd

d322b70

devongovett reviewed Sep 21, 2022

View reviewed changes

LFDanLu added 2 commits September 21, 2022 16:55

Merge branch 'dnd_api_simplification_followups' of github.com:adobe/r…

094aa06

…eact-spectrum into dnd_api_simplification_followups

optimization to default useDrop filtering

4dd87af

moving list types into aria and spectrum packages

a9f5e5a

this avoids the need for adding a dev dep of react-spectrum/dnd to the types packages

LFDanLu commented Sep 22, 2022

View reviewed changes

devongovett mentioned this pull request Sep 22, 2022

Add documentation for drag and drop in React Aria #3423

Merged

devongovett previously approved these changes Sep 22, 2022

View reviewed changes

LFDanLu added 2 commits September 22, 2022 10:24

reexport grid list and spectrum list prop from react-types/list

53b3360

for backcompat. We define and export these props from aria/spectrum packages now, the react-types/list packages is no longer necessary since it is only used for one component

extra space removal

604cc03

LFDanLu dismissed devongovett’s stale review via 604cc03 September 22, 2022 17:26

devongovett approved these changes Sep 22, 2022

View reviewed changes

snowystinger approved these changes Sep 22, 2022

View reviewed changes

Merge branch 'main' into dnd_api_simplification_followups

63961c5

LFDanLu merged commit cb54cf4 into main Sep 22, 2022

LFDanLu deleted the dnd_api_simplification_followups branch September 22, 2022 23:17

LFDanLu added the needs testing label Sep 23, 2022

dannify removed the needs testing label Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update simplified DnD handler api and memoize useDnDHooks #3533

Update simplified DnD handler api and memoize useDnDHooks #3533

LFDanLu commented Sep 19, 2022

LFDanLu Sep 19, 2022 •

edited

Loading

adobe-bot commented Sep 19, 2022

adobe-bot commented Sep 20, 2022

LFDanLu Sep 20, 2022

LFDanLu Sep 20, 2022

LFDanLu Sep 20, 2022

devongovett Sep 21, 2022

devongovett Sep 21, 2022

devongovett Sep 21, 2022

devongovett Sep 21, 2022

LFDanLu Sep 21, 2022

adobe-bot commented Sep 22, 2022

LFDanLu Sep 22, 2022

devongovett Sep 22, 2022

LFDanLu Sep 22, 2022

adobe-bot commented Sep 22, 2022

adobe-bot commented Sep 22, 2022

adobe-bot commented Sep 22, 2022

		@@ -70,9 +70,9 @@ export function useDroppableCollection(props: DroppableCollectionOptions, state:
		let filteredItems = items;
		if (acceptedDragTypes) {

Update simplified DnD handler api and memoize useDnDHooks #3533

Update simplified DnD handler api and memoize useDnDHooks #3533

Conversation

LFDanLu commented Sep 19, 2022

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

LFDanLu Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

adobe-bot commented Sep 19, 2022

adobe-bot commented Sep 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Sep 22, 2022

adobe-bot commented Sep 22, 2022

adobe-bot commented Sep 22, 2022

LFDanLu Sep 19, 2022 •

edited

Loading