-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[HOLD for payment 2022-04-01] [$1000] Long press on image doesn't open the context menu - Reported by @rushatgabhane #7462
Comments
Triggered auto assignment to @tjferriss ( |
Triggered auto assignment to @roryabraham ( |
Triggered auto assignment to @stephanieelliott ( |
Was able to reproduce this on iOS. Posted to Upwork: https://www.upwork.com/jobs/~0109b6b91df96f2613 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
Current assignee @roryabraham is eligible for the Exported assigner, not assigning anyone new. |
@rushatgabhane I have a few questions. |
@parasharrajat good question. Except for edit comment, I'd expect the context menu to have all other options i.e. unread, copy and delete. |
Ok. Thanks for confirming. |
2xd price to $500 |
2xd price to $1000 |
Reason: Image thumbnail is wrapped with in a TouchableOpacity, which captures all touch events at thumbnail level. App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js Lines 47 to 64 in ddae433
Context menu is opened via App/src/pages/home/report/ReportActionItem.js Lines 152 to 157 in 9d6e07a
Since the inner touchable captures the tap event it never reaches the Proposal: Move AttachmentModal logic to ReportActionItem level and control the visibility of the modal from onPress callback of <PressableWithSecondaryInteraction
ref={el => this.popoverAnchor = el}
onPressIn={() => this.props.isSmallScreenWidth && canUseTouchScreen() && ControlSelection.block()}
onPressOut={() => ControlSelection.unblock()}
onPress={this.showAttachmentModal}
onSecondaryInteraction={this.showPopover}
preventDefaultContentMenu={!this.props.draftMessage}
> We could go further ahead and introduce an AttachmentModal route at the RootNavigation level and navigate to the modal on tapping image thumbnail. This way a nested Modal inside each |
Hey @parasharrajat, any feedback on the proposal above? |
@stephanieelliott Sorry, I was sleepy when I reviewed it so couldn't leave a comment.
I don't see how will you do that in your proposal.
It will create more challenges. Do we want to make the attachment URL accessible? I think no. As attachments are related to reports. It would need to be sub route of the report. I don't think this is a good approach. There are other ways. You can take a look at |
@kadiealexander it's in review |
Sorry, the delay here is on me. Catching up now... |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.43-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-03-24. 🎊 |
@kadiealexander Could you please hold C+ payment for me on this issue until I request/ping you. I would like to wait for a week. Please put this on weekly so that you don't get notifications from MelvinBot. Thanks. |
@kadiealexander but it will be good if my payment not hold 😄 Thanks |
@ahmdshrif of course! I'll issue your payment and @rushatgabhane's reporting bonus today. @parasharrajat I'll keep yours on hold, let me know when you're ready. |
Paid:
|
Thanks @kadiealexander |
@parasharrajat how are you tracking for this? Still want me to keep it on hold? |
Nope @kadiealexander. You are good to go. |
@parasharrajat just sent you a contract, please accept! |
@kadiealexander Done. |
Bump @kadiealexander. |
Apologies @parasharrajat, I was ooo! All paid now :) |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here. 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. |
Bot successfully found one true regression but this PR is very old so we have to continue on the other issue. |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here. 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. |
1 similar comment
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here. 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. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Long press on image should open the context menu.
Actual Result:
Long press on image doesn't open the context menu. (Long press on side of image does)
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.1.33-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
screen-20220116-194114.mp4
Expensify/Expensify Issue URL:
Issue reported by: @rushatgabhane
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1642351724013000
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: