-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$500] mWeb/safari - IOU - Problems with selecting the numbers #35436
Comments
Triggered auto assignment to @JmillsExpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~01751b37bd442a3d39 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Unable to adjust selection within amount input in IOURequestMoney screen What is the root cause of that problem?IOURequestStartPage is wrapped in a OnyxNavigator.
OnyxTabNavigator uses MaterialTopTabNavigator under the hood. Default behaviour of TopTabNavigator is to allow switching between the tabs by swiping horizontally across the screen. While trying to adjust the selection within AmountTextInput, swipe gesture is invoked at Navigator level and triggers tab switch. What changes do you think we should make in order to solve the problem?Stop propagation of onTouchMove event at AmountTextInput level. This will make sure that the event doesn't get bubbled up to the TopTabNavigator and the tab switch never gets triggered. onTouchMove={e => e.stopPropagation()} What alternative solutions did you explore? (Optional)Another option is to disable swipe gesture for the entire screen by passing <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL} options={{swipeEnabled: amountSelected ? false : true }}>{() => <IOURequestStepAmount route={route} />}</TopTab.Screen> |
ProposalPlease re-state the problem that we are trying to solve in this issue.When trying to select the amount text, the tab is swiped. What is the root cause of that problem?When we try to select the text, the tab navigator receives the swipe touch too, so the tab is swiped to the next tab. What changes do you think we should make in order to solve the problem?We had this issue on map view too before and what we did was to use a pan responder to intercept the swipe. On our textinput, we already have a shouldInterceptSwipe (added from this PR) that will apply the pan responder, so we can simply pass |
ProposalPlease re-state the problem that we are trying to solve in this issue.Highlighting/selecting numbers in the IOU screen is not possible as the screen will swipe to the next tab. What is the root cause of that problem?The component used for the navigation on the IOU screen is a Material top tab navigator link to component One of the default options "swipeEnabled" allows the swiping gesture between tabs, however the swiping gesture is not ignored in this situation when highlighting text. What changes do you think we should make in order to solve the problem?My solution is to add event handlers to the App/src/components/AmountTextInput.tsx Line 57 in 1b1eeb8
After some testing, the best option seems to be to add I chose onPointer handlers as it works for both web and mobile, and this bug is actually also present on web and other browsers, not just iOS safari. I also did not want to block the gesture altogether, as the What alternative solutions did you explore? (Optional)Considered removing swipe gestures entirely by setting swipeEnabled: false in |
@JmillsExpensify, @thesahindia Huh... This is 4 days overdue. Who can take care of this? |
@bernhardoj's proposal looks good to me! 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR is ready cc: @thesahindia |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
A new PR is ready cc: @thesahindia |
Removing the awaiting payment part of the title given that the original PR was reverted. |
@JmillsExpensify, @francoisl, @bernhardoj, @thesahindia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
The latest PR #36463 was deployed to production yesterday. |
@JmillsExpensify, @francoisl, @bernhardoj, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@JmillsExpensify, @francoisl, @bernhardoj, @thesahindia Eep! 4 days overdue now. Issues have feelings too... |
@JmillsExpensify, @francoisl, @bernhardoj, @thesahindia 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it! |
@JmillsExpensify, @francoisl, @bernhardoj, @thesahindia 10 days overdue. Is anyone even seeing these? Hello? |
The fix has been deployed for 2 weeks, I think this is ready for payment now @JmillsExpensify . |
This issue has not been updated in over 14 days. @JmillsExpensify, @francoisl, @bernhardoj, @thesahindia eroding to Weekly issue. |
This wasn't a regression. If we want we can add a test case. Test steps -
|
This issue has not been updated in over 15 days. @JmillsExpensify, @francoisl, @bernhardoj, @thesahindia eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
Payment summary:
|
Contributor paid out, and regression test suggestion has been posted above. It's pretty specific, so I'm not convinced we need to add a new one. |
I'm going to close out this issue though, as C+ is paid via New Expensify. |
Payment summary: Contributor: @bernhardoj $500 - paid in upwork |
$500 approved for @thesahindia |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.33.0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Issue found when executing PR #35119
Action Performed:
Expected Result:
I should be able to move the selector. The selection should only be visible on the digits I selected
Actual Result:
'm not able use the selector from the right side. It moves a digit and switches to the "Scan" tab. The selection is visible on the new tab for a short time
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6361361_1706644958076.UELM9210.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: