Skip to content
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 2021-12-06] $500 Holding 2 fingers on PDF opens context menu - Reported by: @parasharrajat #5870

Closed
isagoico opened this issue Oct 15, 2021 · 18 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

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:

  1. Navigate to a conversation
  2. Send a PDF
  3. Open the preview
  4. Hold 2 fingers over the PDF

Expected Result:

No context menu should be displayed in attachment preview

Actual Result:

Context menu displayed.

Workaround:

None needed.

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.7-22

Reproducible in staging?: Yes
Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1634141035220700

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @yuwenmemon (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@MelvinBot
Copy link

@yuwenmemon Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@MelvinBot
Copy link

@yuwenmemon Huh... This is 4 days overdue. Who can take care of this?

@yuwenmemon yuwenmemon added the External Added to denote the issue can be worked on by a contributor label Oct 20, 2021
@MelvinBot
Copy link

Triggered auto assignment to @SofiedeVreese (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot removed the Overdue label Oct 20, 2021
@SofiedeVreese SofiedeVreese self-assigned this Oct 20, 2021
@SofiedeVreese
Copy link
Contributor

Job posted on Upwork: https://www.upwork.com/jobs/~010ae2998ab7b91209

@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Oct 20, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Luke9389 (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@Luke9389
Copy link
Contributor

@parasharrajat Do you mean the menu with Copy to clipboard, Mark as unread and Delete comment?

If so, why wouldn't we want these options to show up?

@parasharrajat
Copy link
Member

Not sure why the video wasn't uploaded here https://expensify.slack.com/archives/C01GTK53T8Q/p1634141057220800?thread_ts=1634141035.220700&cid=C01GTK53T8Q.

@Luke9389 according to the Video, the context menu is opening on the Preview Modal which it should not.

@Luke9389
Copy link
Contributor

Oh, I see. That makes more sense. Thanks

@SofiedeVreese SofiedeVreese changed the title Holding 2 fingers on PDF opens context menu - Reported by: @parasharrajat $500 Holding 2 fingers on PDF opens context menu - Reported by: @parasharrajat Oct 27, 2021
@SofiedeVreese
Copy link
Contributor

Price increased to $500

@CamilaRivera
Copy link
Contributor

Proposal

We can wrap the PDF component with a component that prevents the press event from going up in the tree. The press event that bubbles up ends up opening the context menu.

TouchableWithoutFeedback is a good option because it works for catching the event and doing nothing with it, and it doesn't give any visual feedback.

We can wrap the following lines:

<PDF
activityIndicator={<FullScreenLoadingIndicator />}
source={{uri: props.sourceURL}}
style={[
styles.imageModalPDF,
getWidthAndHeightStyle(props.windowWidth, props.windowHeight),
]}
/>

like this:

const PDFView = props => (
    <View style={[styles.flex1, props.style]}>
        <TouchableWithoutFeedback>
            <PDF
                activityIndicator={<FullScreenLoadingIndicator />}
                source={{uri: props.sourceURL}}
                style={[
                    styles.imageModalPDF,
                    getWidthAndHeightStyle(props.windowWidth, props.windowHeight),
                ]}
            />
        </TouchableWithoutFeedback>
    </View>
);

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 1, 2021

Ok, let's give this a try @CamilaRivera. You've got the 🟢 green light 🟢 to go ahead and make a PR.

@SofiedeVreese
Copy link
Contributor

Niiice! I've hired @CamilaRivera via Upwork now.

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 2, 2021
@SofiedeVreese
Copy link
Contributor

SofiedeVreese commented Nov 9, 2021

No updates: it has an open PR.

@SofiedeVreese
Copy link
Contributor

Not overdue: PR is open.

@MelvinBot MelvinBot removed the Overdue label Nov 17, 2021
@SofiedeVreese
Copy link
Contributor

PR merged, waiting for deploy so I can add the payment hold.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 29, 2021
@botify botify changed the title $500 Holding 2 fingers on PDF opens context menu - Reported by: @parasharrajat [HOLD for payment 2021-12-06] $500 Holding 2 fingers on PDF opens context menu - Reported by: @parasharrajat Nov 29, 2021
@botify
Copy link

botify commented Nov 29, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.16-10 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 2021-12-06. 🎊

@SofiedeVreese
Copy link
Contributor

Created a separate job in Upwork to pay out @parasharrajat 's reporting bonus.

Paid out @CamilaRivera + company offsite bonus.

Contributor paid: ✅
Contract cancelled: ✅
Upwork posting removed: ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants