Fix: Pdf preview is not in centered#8474
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
luacmartins
left a comment
There was a problem hiding this comment.
Left a small comment.
Also, could you please add test steps to the PR? It's important so that other people without context can QA these changes.
Other than that, LGTM! Great work @metehanozyurt!
| }, | ||
| PDFView: { | ||
| flex: 1, | ||
| display: 'grid', |
There was a problem hiding this comment.
I think we should add a comment here to avoid setting a bad example. Something like:
`display: grid` is not supported in native platforms! It's being used on Web/Desktop only to vertically center short PDFs, while preventing the overflow of the top of long PDF files.
|
Thank you so much @luacmartins. I will add steps and will add sample PDF documents too. I will fix my mistakes 🙏 . |
There was a problem hiding this comment.
LGTM and tests well! Thanks for the changes! @Santhosh-Sellavel do you wanna take a look before I merge?
|
I was OOO. Thanks, @luacmartins for reviewing it. Looks good to me I would like to test it again once please wait until. Mostly will do it today or by tomorrow. Thanks again! |
PR Reviewer Checklist
|
Santhosh-Sellavel
left a comment
There was a problem hiding this comment.
Looks good to me, tests well! @luacmartins
|
✋ 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 @luacmartins in version: 1.1.55-0 🚀
|
|
@luacmartins iOS Deploy failed here |
|
🚀 Deployed to staging by @luacmartins in version: 1.1.55-0 🚀
|
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.55-2 🚀
|
Details
Pdf preview is in centered.
Fixed Issues
$ #8127
Tests
Test video example:
20220405_220045.mp4
Small size pdf sample:
sampleSmall.pdf
Big size pdf sample:
sampleBig.pdf
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android