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

refactor: make drawPdf function less opinionated #419

Merged
merged 2 commits into from Jan 16, 2024

Conversation

TheNewLad
Copy link
Contributor

@TheNewLad TheNewLad commented Jul 4, 2023

I think drawPdf should leave the styling and scale of the pdf to the hook caller.

Setting the canvas height and width is fine for displaying the PDF since the canvas is taking on the height and width of the PDF. The canvas element that uses the hook can be styled in any fashion through class name or style.

This also provides a solution to #405.


Before this change, the scale was only applied to the pdfjs-generated viewport, this instead applies the scale to the canvas. Referenced from this SOF: https://stackoverflow.com/a/5306929/4771642

I think drawPdf should leave the styling and scale of the pdf to the hook caller.  

Setting the canvas height and width is fine for displaying the PDF since the canvas is taking on the height and width of the PDF. The canvas element that uses the hook can be styled in any fashion through class name or style.

The default scale of 1 is acceptable for any screen. Making the caller of the hook responsible for the scale allows for better usability of the hook in many cases.
@vercel
Copy link

vercel bot commented Jul 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-pdf-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 8, 2023 0:53am

This fixes the scale issue and takes into account the device dpi. Before the scale was only applied to the pdfjs generated viewport. This applies the scale to actual canvas instead. Referenced from this SOF: https://stackoverflow.com/a/5306929/4771642
Copy link
Owner

@mikecousins mikecousins left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

@mikecousins mikecousins merged commit bc386fd into mikecousins:main Jan 16, 2024
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.

None yet

2 participants