Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.42k stars 2.8k forks source link

[New Feature] Not able to zoom pdf @gadhiyamanan #11398

Closed kavimuru closed 11 months ago

kavimuru commented 2 years ago

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. Go to any chat which has pdf attached
  2. Click on pdf and try to zoom pdf

Expected Result:

Should be able to zoom pdf

Actual Result:

Not able to zoom pdf

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.9-0 Reproducible in staging?: Need reproduction Reproducible in production?: Need reproduction Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/192899760-1cc94365-56d6-4eb8-b8cb-9af05c4d8662.mp4

Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663263653930829

View all open jobs on GitHub

gadhiyamanan commented 1 year ago

@kavimuru no one assigned here, I am still able to reproduce it with the latest build

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @slafortune (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

CortneyOfstad commented 1 year ago

Helping Stevie with this and here is a recording of the issue from my NewDot:

2022-10-19_13-00-38 (1)

slafortune commented 1 year ago

@kavimuru - I was also able to reproduce this on mobile web

I was not able to reproduce this on mobile app as shown below Andriod 12 Galaxy Z Flip4

https://user-images.githubusercontent.com/11725910/196771741-fa140fcb-dd8e-4850-94b5-c73fa8369b6b.mp4

Also shown above, command+ does zoom until it hits the page width set.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @slafortune is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@Julesssss, @slafortune, @rushatgabhane Whoops! This issue is 2 days overdue. Let's get this updated quick!

Julesssss commented 1 year ago

Awaiting proposals

slafortune commented 1 year ago

https://www.upwork.com/ab/applicants/1585623107464331264/job-details

rushatgabhane commented 1 year ago

@slafortune time to double?

flodnv commented 1 year ago

I think this is a feature request and not a bug. Do you agree @Julesssss @madmax330 @puneetlath ?

madmax330 commented 1 year ago

I'm not sure. If it's working on all other platforms except this one it sounds like a bug. But if it doesn't work on this platform because we need a platform specific solution then it would be a feature request.

flodnv commented 1 year ago

Ah I missed this is only a bug in mweb 👍 It works for me on Android. I went to test on mweb and it fails to download the PDF file 😬

Julesssss commented 1 year ago

Hey @slafortune could we please double the bounty for this issue? Thanks

slafortune commented 1 year ago

Doubled!

Julesssss commented 1 year ago

Thank, still awaiting proposals.

vladamx commented 1 year ago

Proposal

Problem

What i think is happening here is that we actually don't have any built in zooming for pdf on web.

We are just using react-pdf to display the pdf in a Page(container exported from react-pdf) that always has the same width which gives 1:1 content ratio. No zoom gestures or zoom controls are implemented inside react-pdf itself.

If someone managed to zoom the content in the browser it is just the "zoom all" feature every browser has. We are not actually zooming the pdf on web by any means.

Solution - Scaling the content up

By increasing the width of the Page component exported from react-pdf and having the viewport to scroll we are actually able to scale content arbitrarily by increasing the multiplier for pageWidth either by implementing the control bar or some gesture detection mechanism.

The code snippet below is a brief showcase of changes needed for the content to scale up.

image

Screenshoots/ Video:

Default scale: width = pageWidth

Screenshot 2022-11-15 at 00 28 23

3x scale: width = pageWidth * 3

https://user-images.githubusercontent.com/11381171/201790236-5fb813e9-6a53-4287-b15e-28cd0ec11902.mp4

rushatgabhane commented 1 year ago

we actually don't have any built in zooming for pdf on web

agree

increasing the multiplier for pageWidth either by implementing the control bar or some gesture detection mechanism.

@vladamx that's a really clever workaround! How difficult would it be to implement gestures? Can we import a library and call it a day?

flodnv commented 1 year ago

This feels more and more like a feature request, I don't think it's a bug IMO -- much less a WAQ bug, cc @JmillsExpensify

JmillsExpensify commented 1 year ago

I completely agree. Added that and a HOLD to the title. More context on this decision in Slack.

tjferriss commented 1 year ago

Following instructions for the weekly update chore:

@Julesssss this is one of the oldest issues in the /App repo. To help us clear out the large backlog of bugs, can you:

I recognize this issue is on hold, but once it's not on hold do you expect we should take this internal given the issue's age?

vladamx commented 1 year ago

I think we already have a solution for this issue - probably can be resolved fairly quickly. See https://github.com/Expensify/App/issues/11398#issuecomment-1314551092

flodnv commented 1 year ago

@tjferriss this is not a bug.

Julesssss commented 1 year ago

I'm unassigning as this isn't part of Bug-zero and is on hold for the foreseeable future

JmillsExpensify commented 1 year ago

Updated the title to appropriately reflect that this is held during WAQ.

slafortune commented 1 year ago

Not over due

rushatgabhane commented 1 year ago

@JmillsExpensify hold can be lifted here?

JmillsExpensify commented 1 year ago

Great call, I removed the hold. That said, I think we should be careful about the implementation. I want to avoid an issue/PR where we haven't planned thoroughly and the PR starts to get very tricky.

vladamx commented 1 year ago

@rushatgabhane @JmillsExpensify

I would like if we can expand on my existing proposal above since i have done some initial research about this topic.

Lets define what else is needed for this.

rushatgabhane commented 1 year ago

@vladamx for this feature, we can

  1. Work on refining a proposal on github, and when we think it's good
  2. Post the same on slack, following this format of how employees do pre-design - https://expensify.slack.com/archives/C01GTK53T8Q/p1674586525755509

thoughts? overkill? or more eyes would be worth it here

cc @JmillsExpensify

JmillsExpensify commented 1 year ago

Love that plan @rushatgabhane.

JmillsExpensify commented 1 year ago

I'll help provide any guidance needed when the time comes, assigning myself.

vladamx commented 1 year ago

@rushatgabhane https://expensify.slack.com/archives/C01GTK53T8Q/p1675033719250629

JmillsExpensify commented 1 year ago

One idea for this issue: I think we should put it on hold until ECX. I think potentially everyone on this issue will be in Curacao?

JmillsExpensify commented 1 year ago

We never got to this issue over Curacao. Thoughts about picking it back up now?

rushatgabhane commented 1 year ago

@vladamx you wanna work on this together for the pre-design? We can build off of the feedback we got on slack.

We can discuss 1:1 a bit and I can take the lead if you want

vladamx commented 1 year ago

@rushatgabhane Yeah, lets discuss. I would like to implement this if that's fine. I have some spare time at the moment.

MelvinBot commented 1 year ago

📣 @vladamx! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
vladamx commented 1 year ago

Contributor details Your Expensify account email: vladamx.dev@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~017751bdb1e653662b

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

rushatgabhane commented 1 year ago

no updates yet

JmillsExpensify commented 1 year ago

Still no updates

rushatgabhane commented 1 year ago

same. not a priority ig

JmillsExpensify commented 1 year ago

Same as above.

JmillsExpensify commented 1 year ago

Same

JmillsExpensify commented 11 months ago

I'm going to close this issue in order to focus on other priorities. If we ever get back to this, we can reopen the issue.