Use pdf container width instead of windows width to calculate page width #5775
Conversation
|
@eVoloshchak This change is not reactive to screen dimensions change. When we resize the app PDF does not resize with respect to that. How are you planning to solve that? Also, please add recordings for all platforms and test them. |
I couldn't reproduce this issue during my tests. Could you please specify on which device/platform the problem occurs? Or, if that's possible, attach a video?
Added recordings for mobile web (Android phone, iPhone, iPad), desktop web and desktop app, updated the PR description. |
|
|
||
| return ( | ||
| <View | ||
| style={[styles.PDFView, this.props.style]} | ||
| onLayout={event => this.setState({windowWidth: event.nativeEvent.layout.width})} |
There was a problem hiding this comment.
Ok. So you are replacing the windowWith with View's width but isn't it wrong? It should be the window's width.
There was a problem hiding this comment.
Ok. So you are replacing the
windowWithwith View's width but isn't it wrong? It should be the window's width.
It is PDF container's width.
Ultimately, to calculate the page width we need to know width of the container. In our case container - is this View.
But the problem is, we don't know the container's width until first render is occured. Since container takes up full width, we can use Dimensions.get('window').width as the initial number for container width. After component renders, View's onLayout fires, updating windowWidth value.
But I agree, calling in windowWidth is misleading.
Should we call it something like containerWidth or pdfContainerWidth?
There was a problem hiding this comment.
Yeah, I understand this point. Thanks for the explanation.
Since container takes up full width, we can use Dimensions.get('window').width as the initial number for container width.
But as you said, if we can use the windowWidth which is better as it prevents render. on each onLayout state will cause another render.
So, we can skip this logic and use the Dimensions.get('window').width always. I know this is wrong to do but it fits well in our cause.
There was a problem hiding this comment.
But as you said, if we can use the windowWidth which is better as it prevents render. on each onLayout state will cause another render.
So, we can skip this logic and use the
Dimensions.get('window').widthalways. I know this is wrong to do but it fits well in our cause.
This fix introduces only one additional render when component is first mounted.
If we use Dimensions.get('window').width always, pages won't resize when we resize the window.
onLayout, on the other hand, will fire when window is resized.
There was a problem hiding this comment.
I mean we won't need that state then.
you can directly use the Dimensions.get('window').width in the render method and this will update every time page resizes.
Am I thinking wrong?
There was a problem hiding this comment.
I mean we won't need that state then.
you can directly use the
Dimensions.get('window').widthin the render method and this will update every time page resizes.
That's the problem, when we zoom-in hard enough, Dimensions.get('window').width returns invalid screen size.
I've described this issue in my initial proposal. In the first video you can see alerts popping up when i zoom pdf, windowWidth parameter is the one withWindowDimensions is returning.
There was a problem hiding this comment.
The code looks good. Unfortunately, I was not able to test it on Safari Mobile bcz I got none.
I will leave this to others. cc @AndrewGable.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @AndrewGable in version: 1.1.8-10 🚀
|
|
🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀
|
Details
inconsistent behaviour was caused by Dimensions 'change' listener returning invalid window sizes when zooming in or out.
We've settled on using pdf container's width to determine the page width.
Fixed Issues
$ #4867
Tests
npm run webQA Steps
Same as
TestsTested On
Screenshots
iPhone X running iOS 14.7.1 (Safari)
vert.mp4
horiz.mp4
OnePlus 6 running Android 11 (Chrome)
21-10-12-23-53-24.mp4
Chrome for MacOS
Untitled.mp4
Native MacOS app
Untitled.1.mp4
iPad Pro simulator running iOS 15 (Safari)
Untitled.2.mp4
I've also tested native android app, but that's irrelevant, since changes are made only to PDFView/index.js file, while native applications are using PDFView/index.native.js