-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Migrate DragNDrop #23589
Migrate DragNDrop #23589
Conversation
@fedirjh I'm assigning this to @allroundexperts for review since I came from #23252 |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
@roryabraham I'm getting the following console error. Can you please confirm / fix? Screen.Recording.2023-07-28.at.9.40.22.PM.mov |
@allroundexperts I'm not seeing the same: web.movTried 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 |
Not in-scope for this PR but this code should really listen for |
Gotcha. Testing again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Going to merge this because it's blocking another pretty urgent PR |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@roryabraham I see this on the latest iOS/android main. Possibly related to this PR or am I doing something wrong? |
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 @roryabraham I can raise a quick fix PR for this if you're busy. |
PR: #23829 |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.48-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.48-5 🚀
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.49-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.49-3 🚀
|
shouldAllowDrop: false, | ||
}); | ||
return ( | ||
<View |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -36,6 +36,7 @@ function DragAndDropProvider({children, dropZoneID, dropZoneHostName, isDisabled | |||
const {windowWidth, windowHeight} = useWindowDimensions(); | |||
|
|||
const dropZone = useRef(null); | |||
const dragCounter = useRef(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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:
A simplified version of how it was used before looks something like this:
in the parent:
In the child:
This PR makes it a bit simpler to use. Now it looks something like this:
in the parent:
in the child:
Fixed Issues
$ #16142
Tests (web/desktop only)
Web-PDFs/var/pdfs.expensify.com
and runcomposer install
+
button in the composer.Offline tests
Verify that you can drag and drop a file to add it as an attachment.
QA Steps (web/desktop only)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
storybook.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
desktop.mov
iOS
Android