Add DraggableList component #26307
Add DraggableList component #26307
Conversation
|
@kosmydel please let me know the ETA for this issue 🙇 |
Hey, @blazejkustra took this over as I'm participating in internal workshops and have limited availability. It should be ready for a review today. |
|
@situchan @Li357 Thanks @blazejkustra for the support! |
|
@situchan Are you able to review this soon? |
There was a problem hiding this comment.
Overall looks good. +1 on @fabioh8010's feedback
Improvement suggestions: (not blocker but can be follow-up)
BUG1: (web) hover glitch after drop
bug1.mov
BUG2: [iOS] too much padding (I think this is blocker so should be fixed here)
BUG3: Inconsistent hover color between platforms
i.e. on web, there's highlight color while dragging but not on native
bug3.mov
BUG4: [mSafari] sometimes full page is selected (blue highlight)
bug4.mov
BUG5: [mChrome] long press threshold - while scrolling, sometimes drag started though I didn't long-press
bug5.mov
hayata-suenaga
changed the title
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.78-0 🚀
|
mountiny
removed
the
DeployBlockerCash
| isActive={isActive} | ||
| getIndex={getIndex} | ||
| onPress={navigateToWaypointEditPage} | ||
| disabled={isLoadingRoute} |
There was a problem hiding this comment.
@blazejkustra Sorry. I have a minor question. Please help answer when you have a chance. Why do we need this logic?
There was a problem hiding this comment.
@DylanDylann This line disables the items of draggable list when the route is reloading, so that it is impossible to reorder the list while the map is reloading. Without it, there is a possible race condition if you change the order of items quickly while the map is still loading the route.
There was a problem hiding this comment.
Many thanks @blazejkustra
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
| transaction: { | ||
| key: ({transactionID}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID || 0}`, | ||
| }, | ||
| mapboxAccessToken: { |
There was a problem hiding this comment.
I think this may have been missed during the refactoring, but this prop is no longer accessed in this component.
There was a problem hiding this comment.
yes, that can be removed as unused. Moved to DistanceRequestFooter
There was a problem hiding this comment.
That's right. I will prepare a PR for that tomorrow.
There was a problem hiding this comment.
Added this change to this PR.
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀
|
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.79-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
| } | ||
| }, [stop]); | ||
| - var bindCapturingEvents = useMemoOne.useCallback(function bindCapturingEvents() { | ||
| + var bindCapturingEvents = useMemoOne.useCallback(function bindCapturingEvents(target) { |
There was a problem hiding this comment.
Hi @kosmydel . I am working on "Create a fork for react-beautiful-dnd and remove patch" and I need to define a type for target param. So what should it be? Expensify/react-beautiful-dnd#1 (comment)
There was a problem hiding this comment.
Hey @DylanDylann, thanks for the question!
I think it will be HTMLElement.
Here is the original PR, with this change. And it is already typed.
| waypoint0: null, | ||
| waypoint1: null, |
There was a problem hiding this comment.
This change has caused a regression explained here
#29748 (comment)
|
|
||
| setOptimisticWaypoints(newWaypoints); | ||
| // eslint-disable-next-line rulesdir/no-thenable-actions-in-views | ||
| Transaction.updateWaypoints(transactionID, newWaypoints).then(() => { |
There was a problem hiding this comment.
Looks like dragging the waypoint to empty waypoint duplicates it which caused #33919
Details
Implement a
DraggableListfor waypoints.Fixed Issues
$ #22716
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
android-web.mov
Mobile Web - Safari
ios-web.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov