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

PDF preview component #2

Merged
merged 35 commits into from
Nov 6, 2023

Conversation

rezkiy37
Copy link
Contributor

@rezkiy37 rezkiy37 commented Sep 20, 2023

Details

This is a PR where we introduce a MVP version of react-fast-pdf package. The package codebase itself has a basic configuration to read and preview a PDF file. The example has a basic template how to use the package.

Related Issues

Expensify/App#22998

Manual Tests

  1. Clone the repo.
  2. Install deps. for both package and example: npm install && cd example && npm install && cd ...
  3. Run a script - cd example && npm run start && cd .. - of the example.
  4. Verify that Webpack launches successfully.
  5. Verify that it opens a tab.
  6. Verify that the PDF previewer works.

Linked PRs

N/A

Screenshot 2023-09-20 at 19 51 08
PDF.mp4

@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@rezkiy37
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA.

@rezkiy37
Copy link
Contributor Author

By the way, I've tried to connect the package to the Expensify (NewDot) app. It works.

Chrome.mp4
Desktop.mp4

@allroundexperts
Copy link

@rezkiy37 Has this approach merging dist until the package is deployed validated somewhere? I think its not a good idea to track the built files. We've already merged 1 pull request and it should be enough for us to publish the package. I would rather put this on hold until we publish instead of committing build files.

@rezkiy37
Copy link
Contributor Author

@allroundexperts, we had a discussing about it in the issue.
You approved this as a temporary solution - Expensify/App#22998 (comment).

@rezkiy37
Copy link
Contributor Author

@allroundexperts, how can we move forward?

@allroundexperts
Copy link

@rezkiy37 I'm sort of re-thinking this again. We had the same discussion here as well (which is another new package that we're forking).

@allroundexperts
Copy link

@rezkiy37 Do we have any issue to track the publishing of this package to npm?

@rezkiy37
Copy link
Contributor Author

@rezkiy37 Do we have any issue to track the publishing of this package to npm?

Who can help us from the Expensify team with access to their NPM?

@allroundexperts
Copy link

Who can help us from the Expensify team with access to their NPM?

I think its the infra team. Would be best to ask in the open source channel.

@allroundexperts
Copy link

Tested and confirmed that its working well!

Screen.Recording.2023-11-06.at.11.10.36.PM.mov

Copy link

@allroundexperts allroundexperts left a comment

Choose a reason for hiding this comment

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

Tests well!

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks, I can move it ahead and hopefully we can get the npm setup here soon

@mountiny mountiny merged commit bde3fa4 into Expensify:main Nov 6, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants