Migrate DragNDrop by roryabraham · Pull Request #23589 · Expensify/App

roryabraham · 2023-07-25T20:52:43Z

Details

Migrating DragNDrop to a functional component. The existing configuration of DragAndDrop we had was pretty circuitous and also didn't really follow normal React patterns. Before, it was implemented something like this:

In ReportScreen, we pass an ID to a view. That's the view we're listening for drag events
Also in ReportScreen, we set up a PortalHost which will be used to later display the UI that appears when you do drag a file over
Then in ReportActionCompose, we set up the listeners for drag events on that view in the ReportScreen, and save the drag state to ReportActionCompose state
Then in ReportActionCompose, if the parent view is dragged over, we render a UI into the PortalHost we set up in the ReportScreen
There was also some confusing stuff with three different props to identify a "drop zone":
- dropZoneID
- activeDropZoneID
- dropZoneHostName
In order to use it, you had to set up:
- parent view w/ nativeID
- PortalHost
- DragAndDrop – use callbacks and manage state manually
- DropZone (i.e: the Portal "client")

A simplified version of how it was used before looks something like this:

in the parent:

<View nativeID={dropZoneID}>
    ...
    ...
    <PortalHost hostName={dropZoneHostName} />
</View>

In the child:

const [isDraggingOver, setIsDraggingOver] = useState(false);

...

/* Note: technically there's no point in putting children inside this component... */
<DragAndDrop
    dropZoneID={dropZoneID}
    onDragEnter={() => setIsDraggingOver(true)}
    onDragLeave={() => setIsDraggingOver(false)}
    onDrop={() => { /* do callback */ }}
/>

...

isDraggingOver && {
    <Portal hostName={dropZoneHostName}>
        {/* There were some helper components, but essentially here is where we render what appears when the parent view w/ dropZoneID is hovered over */}
    </Portal>
}

This PR makes it a bit simpler to use. Now it looks something like this:

in the parent:

<DragAndDropProvider dropZoneID={dropZoneID}>
    {/* Render children normally here, developer doesn't have to create a Portal */}
</DragAndDropProvider>

in the child:

<DragAndDropConsumer dropZoneID={dropZoneID}>
    {
    /*
      * Here is where you render what appears when the parent view w/ dropZoneID is hovered over.
      * It will automatically only render when the dropZoneID is dragged over
      * It will be absolute-positioned w/ the same dimensions and position in window as the parent `dropZoneID`. You can style the content relative to that.
      */
    }
</DragAndDropConsumer>

Fixed Issues

$ #16142

Tests (web/desktop only)

If you are using an Expensify dev DM, vssh into the VM, navigate to Web-PDFs/var/pdfs.expensify.com and run composer install
Open a report and verify that you can send a message as normal, hover over items, and click on the + button in the composer.
Drag a file over the composer.
Verify that you can drag and drop a file to add it as an attachment.
Drag a file in and out of the report view, verify that the "drag hover state" toggles off and on correctly.
Open the settings page. Try to drag-and-drop a file, verify that dropping the file does nothing and does not open up the image in a new browser tab.
Close and reopen the settings page, then repeat the previous step and make sure the behavior hasn't changed.
Now open another report, and verify that you can drag-and-drop to upload an image.
Run storybook, make sure that the stories for DragAndDrop are still working.

Verify that no errors appear in the JS console

Offline tests

Verify that you can drag and drop a file to add it as an attachment.

QA Steps (web/desktop only)

Verify that you can drag and drop a file over the composer to add it as an attachment.
Go to https://staging.new.expensify.com/docs/index.html and look at the story for DragAndDrop. Make sure it looks and functions as expected by dragging a file over it.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

storybook.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

desktop.mov

iOS

Android

melvin-bot · 2023-07-25T21:45:13Z

@fedirjh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

roryabraham · 2023-07-25T21:46:03Z

@fedirjh I'm assigning this to @allroundexperts for review since I came from #23252

OSBotify · 2023-07-25T22:05:22Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/23589/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/23589/index.html

desktop 💻	web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/23589/NewExpensify.dmg	https://23589.pr-testing.expensify.com

…menting counter

allroundexperts · 2023-07-28T16:42:04Z

@roryabraham I'm getting the following console error. Can you please confirm / fix?

Screen.Recording.2023-07-28.at.9.40.22.PM.mov

roryabraham · 2023-07-28T18:23:51Z

@allroundexperts I'm not seeing the same:

web.mov

Tried with a PDF as well, didn't see the error. Also verified that the global create popover is correctly closed when you drag over:

web.mov

roryabraham · 2023-07-28T18:28:23Z

Not in-scope for this PR but this code should really listen for dragenter instead of dragover since dragover is a continuous event that gets fired many times per second

allroundexperts · 2023-07-28T18:49:11Z

Gotcha. Testing again!

allroundexperts

Tests well @roryabraham. I wasn't able to find any other thing. That being said, given the size of this PR, I think it will be beneficial if we can get another pair of eyes on this (If this is not super urgent).

melvin-bot · 2023-07-28T19:24:28Z

@Li357 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

roryabraham · 2023-07-28T19:43:23Z

Going to merge this because it's blocking another pretty urgent PR

OSBotify · 2023-07-28T19:48:12Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

luacmartins · 2023-07-28T20:41:14Z

@roryabraham I see this on the latest iOS/android main. Possibly related to this PR or am I doing something wrong?

allroundexperts · 2023-07-28T21:05:59Z

I just built the app on native and saw that it loaded fine. Should have tested composer as well. Anyways, I think this is the culprit. We need to return null as far as I can see.

@roryabraham I can raise a quick fix PR for this if you're busy.

AndrewGable · 2023-07-28T21:14:03Z

PR: #23829

OSBotify · 2023-07-31T18:57:47Z

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.48-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-08-01T23:59:03Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.48-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-08-02T01:23:33Z

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.49-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-08-03T18:16:00Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.49-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

allroundexperts · 2023-08-11T07:27:47Z

+        shouldAllowDrop: false,
+    });
+    return (
+        <View


The wrapping View caused this. When creating a new workspace in ThreePaneView component, props.state.routes still contains NAVIGATORS.RIGHT_MODAL_NAVIGATOR route. This causes the App to be wrapped in NoDropZone even after RHN is closed.
Mentioning it here so that it remains documented.

s77rt · 2024-01-22T00:57:24Z

    const {windowWidth, windowHeight} = useWindowDimensions();

    const dropZone = useRef(null);
+    const dragCounter = useRef(0);


If we drag on an element the counter will increase. If that element is removed from dom, the counter will not decrease (stale date). This caused #33820

roryabraham added 5 commits July 25, 2023 13:27

Rename dropZoneID prop to follow conventions

65cac31

Add prop types on default components

9851ad1

Rename disabled prop to isDisabled

f5f8743

Create useEffectOnPageLoad hook

faa216b

Convert DragAndDrop to functional component

931043b

roryabraham self-assigned this Jul 25, 2023

roryabraham added 3 commits July 25, 2023 14:27

Handle dragOver event to fix onDrop

7665425

Fix passing throttled function to useEffectOnPageLoad

401e2c6

Fix yet another whoops with the throttled function

1918bfa

roryabraham added the Ready To Build label Jul 25, 2023

roryabraham marked this pull request as ready for review July 25, 2023 21:45

roryabraham requested a review from a team as a code owner July 25, 2023 21:45

melvin-bot bot requested review from fedirjh and removed request for a team July 25, 2023 21:45

roryabraham requested review from allroundexperts and removed request for fedirjh July 25, 2023 21:45

roryabraham mentioned this pull request Jul 25, 2023

Add ability to Take/Upload Receipts #23046

Merged

56 tasks

roryabraham requested a review from AndrewGable July 26, 2023 01:22

allroundexperts reviewed Jul 27, 2023

View reviewed changes

Comment thread src/hooks/useEffectOnPageLoad/index.js Outdated

roryabraham added 8 commits July 26, 2023 22:51

Re-implement DragAndDrop with context

e0bd6f7

Create useThrottledEffect hook

6be2fe0

remove unused import

23f2d4f

Add flex 1 style to fix display

5949c3e

Fix overlay style

2094ca0

Remove throttled effect hook so measure can happen onLayout

f5afe54

Fix drag state by nesting portal inside transparent overlay and imple…

989f5fa

…menting counter

Simplify by removing measurement

c9a1680

allroundexperts approved these changes Jul 28, 2023

View reviewed changes

melvin-bot bot requested a review from Li357 July 28, 2023 19:24

roryabraham merged commit 5132a6e into main Jul 28, 2023

roryabraham deleted the Rory-MigrateDragNDrop branch July 28, 2023 19:43

luacmartins mentioned this pull request Jul 28, 2023

Fix expense report total when deleting requests #23822

Merged

58 tasks

AndrewGable mentioned this pull request Jul 28, 2023

Return null for native Drag and Drop consumers to prevent Drag UI on native #23829

Merged

56 tasks

gadhiyamanan mentioned this pull request Jul 29, 2023

[$250] Migrate DragAndDrop to function component #16142

Closed

1 task

c3024 mentioned this pull request Jul 29, 2023

[HOLD for payment 2023-08-21] [$1000] Web -Pointer cursor is displayed on send button even when compose box is empty #23848

Closed

6 tasks

This was referenced Jul 29, 2023

[HOLD for payment 2023-09-04] [$1000] Not allowed sign does not show when an attachment is drag and dropped #22218

Closed

[HOLD for payment 2023-08-08] [$1000] Dev: Web - Can't click plus button after leaving a room #23881

Closed

OSBotify mentioned this pull request Jul 31, 2023

Deploy Checklist: New Expensify 2023-07-31 #23945

Closed

48 tasks

marcaaron mentioned this pull request Jul 31, 2023

Not Allowed sign is not shown when dragging attachments on LHN #23969

Closed

6 tasks

OSBotify mentioned this pull request Aug 1, 2023

Deploy Checklist: New Expensify 2023-08-01 #24028

Closed

79 tasks

allroundexperts reviewed Aug 11, 2023

View reviewed changes

s77rt reviewed Jan 22, 2024

View reviewed changes

s77rt mentioned this pull request Jan 22, 2024

[Payment due 1-25][$250] Scan - "Drop file here" modal does not dismiss when file is no longer dragged over the area #33820

Closed

6 tasks

Conversation

roryabraham commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests (web/desktop only)

Offline tests

QA Steps (web/desktop only)

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jul 25, 2023

Uh oh!

roryabraham commented Jul 25, 2023

Uh oh!

OSBotify commented Jul 25, 2023

Uh oh!

Uh oh!

allroundexperts commented Jul 28, 2023

Uh oh!

roryabraham commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roryabraham commented Jul 28, 2023

Uh oh!

allroundexperts commented Jul 28, 2023

Uh oh!

allroundexperts left a comment

Choose a reason for hiding this comment

Uh oh!

melvin-bot bot commented Jul 28, 2023

Uh oh!

roryabraham commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OSBotify commented Jul 28, 2023

Uh oh!

luacmartins commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allroundexperts commented Jul 28, 2023

Uh oh!

AndrewGable commented Jul 28, 2023

Uh oh!

OSBotify commented Jul 31, 2023

Uh oh!

OSBotify commented Aug 1, 2023

Uh oh!

OSBotify commented Aug 2, 2023

Uh oh!

OSBotify commented Aug 3, 2023

Uh oh!

allroundexperts Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

s77rt Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

roryabraham commented Jul 25, 2023 •

edited

Loading

roryabraham commented Jul 28, 2023 •

edited

Loading

roryabraham commented Jul 28, 2023 •

edited

Loading

luacmartins commented Jul 28, 2023 •

edited

Loading

s77rt Jan 22, 2024 •

edited

Loading