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

Desktop: fixes #7932: PDF preview Pane does not show the whole first page. #8192

Closed
wants to merge 1 commit into from
Closed

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented May 15, 2023

Fixes #7932

Problem:

The pdf rendering only assumes the min-height set for it, and wouldn't increase in height even if we have more room for it to grow.

Solution:

Instead of using javascript to handle resizing as suggested by TahaNw, I used CSS vh set to 80 to ensure a part of the note are still visible, and set A4 aspect ratio to ensure pdf scale accordingly

Updated:

view=fit has been added to ensure responsiveness, and min-height=35rem removed to ensure the preview pane does not contain horizontal blank space when on a small width device.

Old Loom video: https://www.loom.com/share/52dffc8a33a949558b3cf39da1db6e58
New Loom video: https://www.loom.com/share/43ea26571c044d1293fe252b810f018a


This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

@gitstart gitstart marked this pull request as ready for review May 15, 2023 07:57
@laurent22
Copy link
Owner

How does it work when the window is scrolled? How does it work when the layout is changed and, for example, the note list is above the preview pane?

@gitstart
Copy link
Contributor Author

Hi @laurent22
It works quite well.

How does it work when the window is scrolled?

when the window is scrolled, the preview pane scroll along. and if you are to scroll through the pages of the pdf, scrolling inside the preview pane handles that.

How does it work when the layout is changed?

It responds to the layout change accordingly maintaining the aspect-ratio with min-height set to 35rem as before

@laurent22
Copy link
Owner

I don't feel the way it works in this PR is much better than before. For example the PDF text remains at a fixed size even when the window size increases.

How do other apps display PDF files? For example Evernote, OneNote, etc.? If it works well in these apps perhaps we can do something similar

@gitstart
Copy link
Contributor Author

gitstart commented May 17, 2023

I see what you mean @laurent22
Okay, i will take a look and update the PR soon, once fixed.

Co-authored-by: Kolade Adetoyinbo <koolcollins25@gmail.com>
@gitstart
Copy link
Contributor Author

Hi @laurent22
Here is the update added

Updated:

view=fit has been added to ensure responsiveness, and min-height=35rem removed to ensure the preview pane does not contain horizontal blank space when on a small width device.

Loom video: https://www.loom.com/share/43ea26571c044d1293fe252b810f018a

@laurent22
Copy link
Owner

Thanks but I don't think it really addresses the original issue. For example, see the example below:

image

The page is fully in view, yes, but it's needlessly small because there's plenty of white space below the PDF viewer. So I think the PDF viewer should take as much vertical space as possible.

@gitstart
Copy link
Contributor Author

gitstart commented Jun 1, 2023

We understand what you mean @laurent22

But what happens when there are some text right below the pdf media?

if we make the pdf viewer take up the entire vertical space, I believe that could lead to some bad user experience, as they may not know there are still text below the pdf viewer.

and also around scrolling, when the pdf viewer fills up the entire vertical space, it will be a bit difficult to scroll the note page, until the pdf viewer's page (if it is multi-pages pdf) has been fully scrolled to the last page.

In conclusion, reviewing the thread, I believe this did address the issue. However we are more than happy to update as you deem fit.

@laurent22
Copy link
Owner

There was also the question:

How do other apps display PDF files? For example Evernote, OneNote, etc.? If it works well in these apps perhaps we can do something similar

Before implementing something, it's important to check what other apps are doing. They already went through the same questions that we have and found answers, so it would be a good idea to compare.

@gitstart
Copy link
Contributor Author

gitstart commented Jun 6, 2023

OneNote's rendering process is somehow complex, however, for evernote i am certain they use @react-pdf/renderer which we can also leverage on here.

In fact we should be able to close this issue: #8277 with it.

@tessus tessus added the desktop All desktop platforms label Jun 27, 2023
@laurent22 laurent22 closed this Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDF preview Pane does not show the whole first page.
3 participants