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.

Sign up for GitHub

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

Jump to bottom

Simplify the PDF renderer for the Web #1521

Merged
merged 6 commits into from
Feb 25, 2021
Merged

Simplify the PDF renderer for the Web #1521

merged 6 commits into from
Feb 25, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Feb 19, 2021 •
edited
Loading

@marcaaron Please review.

Details

You have to apply this fix to see the pdf in local setup #1509

  1. Replaced the old Webview and PDF.js based pdf renderer to the react-pdf lib.
  2. Uninstalled web-view.
  3. Removed the vendor files of the pdf.js.

Fixed Issues

Fixes #1383 Fixes #1514

Tests

Send a Pdf in the chat and click it to see the attachment modal.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshot

Web

image

Mobile Web

image

@parasharrajat parasharrajat requested a review from a team as a code owner February 19, 2021 18:24
@botify botify requested review from MariaHCD and removed request for a team February 19, 2021 18:24
@parasharrajat
Copy link
Member Author

parasharrajat commented Feb 19, 2021 •
edited
Loading

Let me know any styling-related feedback. I added some based on my observations. the Components accepts the className prop but I am not sure how to pass the style prop to the className so I have added a global CSS file.

CC: @shawnborton

@parasharrajat parasharrajat changed the title Parasharrajat/pdf render Simplify the PDF renderer for the Web Feb 19, 2021
marcaaron
marcaaron requested changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice start. We'll need @shawnborton to help explain what kinds of UI additions we need (if any).

src/components/PDFView/index.js Outdated Show resolved Hide resolved
assets/css/pdf.css Show resolved Hide resolved
@shawnborton
Copy link
Contributor

This looks pretty great to me. Do we have control over the color that is used as the background gray behind the PDF content?

@parasharrajat
Copy link
Member Author

@shawnborton. Yes, I have tried to keep all the functionality the same. so we were passing a style prop previously which can set the styles which take gray as default.

@shawnborton
Copy link
Contributor

Great, could we see this with colors.gray1 and colors.gray2 as the background?

@parasharrajat
Copy link
Member Author

@shawnborton

Gray 1 Gray 2 Gray 3
Screenshot_2021-02-23 Expensify cash - Chat with friends to send and receive money(2) Screenshot_2021-02-23 Expensify cash - Chat with friends to send and receive money(1) Screenshot_2021-02-23 Expensify cash - Chat with friends to send and receive money

|

@parasharrajat
Copy link
Member Author

@shawnborton Which one do you like? I would say gray2 is looking best. Just added a commit for text selection on PDF.
@marcaaron I think we are good to go if @shawnborton is fine with the color.

@shawnborton
Copy link
Contributor

I do like gray1 but I can see how it is so light that it doesn't really communicate a background separation the way that gray2 does - so that being said, let's go with gray2

@parasharrajat
Copy link
Member Author

Is something remaining on this PR?

@marcaaron
Copy link
Contributor

yep, will give another review once @shawnborton OK the design we've got.

@shawnborton
Copy link
Contributor

Yup, I'm good with the design and with gray2

marcaaron
marcaaron requested changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just have a couple comments. One thing I've noticed is that the PDF page resizes well when we are on small screens but doesn't look great when we are just transitioning from small to large

Screen Shot 2021-02-23 at 11 34 26 AM

assets/css/pdf.css Show resolved Hide resolved
src/components/PDFView/index.js Outdated Show resolved Hide resolved
src/components/PDFView/index.js Outdated Show resolved Hide resolved
src/components/PDFView/index.js Outdated Show resolved Hide resolved
src/styles/variables.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

Updated. @marcaaron Please review.

marcaaron
marcaaron approved these changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 awesome work on this one!

@marcaaron marcaaron merged commit fb8408d into Expensify:master Feb 25, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2021
@parasharrajat parasharrajat deleted the parasharrajat/pdf-render branch November 4, 2022 19:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marcaaron marcaaron marcaaron approved these changes

@MariaHCD MariaHCD Awaiting requested review from MariaHCD

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Typo in passing props to PDF in PDFView component Simplify the PDF Viewer on Web
3 participants
@parasharrajat @shawnborton @marcaaron