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

App - Unable to scroll the PDF attachment with 2 pages (pay on 22/6) #3247

Closed
isagoico opened this issue May 31, 2021 · 11 comments · Fixed by #3487
Closed

App - Unable to scroll the PDF attachment with 2 pages (pay on 22/6) #3247

isagoico opened this issue May 31, 2021 · 11 comments · Fixed by #3487
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented May 31, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

User should be able to view the whole document.

Actual Result:

User is unable to scroll the PDF file.

Action Performed:

  1. Launch the app
  2. Log in with any account and select any user
  3. Click on add attachment
  4. Upload a PDF with minimum 2 pages
  5. Click on the PDF preview and scroll pages

Workaround:

No workaround found.

Platform:

Where is this issue occurring?

Web
iOS
Android ✔️
Desktop App
Mobile Web

Version Number: 1.0.58-0

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

Notes/Photos/Videos:

Bug5093686_Screen_Recording_20210531-135522_Expensifycash.mp4

Expensify/Expensify Issue URL:

Upwork posting

@MelvinBot
Copy link

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

@Gonals
Copy link
Contributor

Gonals commented Jun 1, 2021

Changing to weekly and sending to the pool!

@Gonals Gonals removed their assignment Jun 1, 2021
@isagoico
Copy link
Author

isagoico commented Jun 7, 2021

Issue reproducible today during KI retests

@arielgreen arielgreen added the External Added to denote the issue can be worked on by a contributor label Jun 7, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 7, 2021
@rdjuric
Copy link
Contributor

rdjuric commented Jun 8, 2021

Proposal

We use react-native-modal for our Modals. In this library, there's a known issue where using a ScrollView inside a swipeable Modal causes that Modal to ignore the scroll movement (react-native-modal/react-native-modal#236).

Our PDF component (from react-native-pdf) is a ScrollView. So when we try to use it inside our Modal component we run into the issue mentioned above.

The workarounds mentioned in that issue involve wrapping the content of our ScrollView inside a TouchableOpacity or a View with a custom lifecycle for the gesture responder. In our case, since the ScrollView is added by a library, this would require a patch to the library.

I think a better solution would be to make our PDF Modals not swipeable. It's already the case that our PDF Modals can't be closed by swiping (since the swipe movements are captured by the PDF), so making it not swipeable would fix the bug without changing any other behavior.

A good way to do this would be creating a new MODAL_TYPE in /src/styles/getModalStyles.js, for our PDF Modals. It would be the same as the current one, but without the swipeDirection.

@MelvinBot
Copy link

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

@iwiznia
Copy link
Contributor

iwiznia commented Jun 8, 2021

@rdjuric that proposal sounds good (assuming it actually works 😄 )
@bfitzexpensify can you hire in upwork?

@rdjuric
Copy link
Contributor

rdjuric commented Jun 9, 2021

@iwiznia Cool! I went ahead and made a PR #3487

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@bfitzexpensify
Copy link
Contributor

Reopening so I remember to pay this out in seven days if there are no regressions.

@bfitzexpensify bfitzexpensify added Weekly KSv2 and removed Daily KSv2 labels Jun 15, 2021
@bfitzexpensify bfitzexpensify changed the title App - Unable to scroll the PDF attachment with 2 pages App - Unable to scroll the PDF attachment with 2 pages (pay on 22/6) Jun 15, 2021
@bfitzexpensify
Copy link
Contributor

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants