[HOLD for payment 2022-04-27] [$500] Pdf preview is not in centered - reported by @thesahindia

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to Chat
2. Try sending the pdf given in thread
3. See if it's in center

Expected Result:

Preview should be in center.

Actual Result:

Preview is not in centered

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.42-3 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Screenshot 2022-03-02 at 12.16.41 AM.pdf

Issue reported by: @thesahindia Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1646160975698469

Job Post: https://www.upwork.com/jobs/~0154111729e0a5dbd4

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @kevinksullivan (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

[MelvinBot]

Triggered auto assignment to @luacmartins (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[kevinksullivan]

@thesahindia please apply when you get a chance!

[kevinksullivan]

Price doubled.

[metehanozyurt]

Proposal

Cause

View doesn't center its children. Simple because it has no proper styling.

Solution

1. We should get total height of the loaded pages at onLoadSuccess callback of the Page component:

   onPageLoadSuccess = (index) => {  
   if (index + 1 === this.state.numPages) {  
       const canvasElements = document.getElementsByClassName('react-pdf__Page__canvas');  
   
       let totalHeight = 0;
   
       // I edit this because getting error when using forEach
       //canvasElements.forEach((ce) => {  
       //    totalHeight += parseInt(ce.style.height, 10);  
       // });  
       for (let counter = 0; counter < canvasElements.length; counter++) {
             totalHeight += parseInt(canvasElements[counter].style.height, 10);
       }
   
       this.setState({canvasHeight: totalHeight});  
   }  
   }

2. We should get the container height at onLayout callback of the View component:

   onLayout = (event) => {  
   if (!(  
       this.state.windowWidth !== event.nativeEvent.layout.width  
   && this.state.windowHeight !== event.nativeEvent.layout.height  
   )) {  
       return;  
   }  
   
   this.setState({windowWidth: event.nativeEvent.layout.width, windowHeight: event.nativeEvent.layout.height});  
   }

3. We should calculate the view style before render if total height of the pages smaller than container height:

   const smallCanvasStyle = (this.state.windowHeight > this.state.canvasHeight) ? {  
   //marginTop: ((this.state.windowHeight - this.state.canvasHeight) / 2) - 10,  
   //marginBottom: ((this.state.windowHeight - this.state.canvasHeight) / 2) - 10,  
   alignItems: 'center',
   } : {};

4. We should pass the calculated style to the View:

   <View  
   style={[styles.PDFView, this.props.style, smallCanvasStyle]}  
   onLayout={this.onLayout}  
   >

https://user-images.githubusercontent.com/6093207/160303949-1fae53ea-baf1-4e81-ac11-bf25c35b4f16.mp4

[luacmartins]

@metehanozyurt It seems like the gray background was modified in your solution. According to the original posting, only the preview content should be affected. I think the solution here is as simple as adding alignItems: 'center' to the PDFView class.

[Santhosh-Sellavel]

Thanks, @luacmartins. I'll review this again tomorrow.

@metehanozyurt Please add an updated proposal address this one https://github.com/Expensify/App/issues/8127#issuecomment-1081005910

[metehanozyurt]

Thanks, @Santhosh-Sellavel. Thank you for correcting me @luacmartins. My solution regarding marginTop and marginBottom is not as good as @luacmartins' which is more accurate for smaller PDF sizes.

However if alignItems: 'center' is added after line 1690, it looks like as the following;

https://github.com/Expensify/App/blob/9c50db754bc29b340f7f725059d0cca6771fc61f/src/styles/styles.js#L1682-L1691

It works flawlessly for PDFs with smaller sizes but if Big PDF Size is selected, the result is not perfect as the prior.

https://user-images.githubusercontent.com/6093207/160481033-cb0d4d7a-0059-4cae-817b-4655ea5b1bd5.mp4

So, if my proposal is edited as below, it seems to be working well.

const smallCanvasStyle = (this.state.windowHeight > this.state.canvasHeight) ? {  
    marginTop: ((this.state.windowHeight - this.state.canvasHeight) / 2) - 10,  
    marginBottom: ((this.state.windowHeight - this.state.canvasHeight) / 2) - 10,  
} : {};

TO

const smallCanvasStyle = (this.state.windowHeight > this.state.canvasHeight) ? {  
    alignItems: 'center',
} : {};

After @luacmartins' suggestion, it looks like as the following:

https://user-images.githubusercontent.com/6093207/160481074-771b9c06-7998-476e-ba3b-57130e65f892.mp4

[kevinksullivan]

Proposal in review

[Santhosh-Sellavel]

Reviewing this 👀

[Santhosh-Sellavel]

@metehanozyurt is this your updated proposal?

[Santhosh-Sellavel]

@metehanozyurt

For smaller pdf, it works well but for bigger pdf, the start of the pdf is not displayed.

https://user-images.githubusercontent.com/85645967/160902307-aba52ce4-563c-4cd8-b04e-4245359b4cb8.mov

cc: @luacmartins

[luacmartins]

I wonder if there's a css only solution for this, since that would be much simpler than playing around with the screen/item sizes.

[Santhosh-Sellavel]

Yep would be nice & clean!

[metehanozyurt]

@metehanozyurt

For smaller pdf, it works well but for bigger pdf, the start of the pdf is not displayed.

Screen.Recording.2022-03-30.at.11.37.24.PM.mov cc: @luacmartins

Thanks for response @Santhosh-Sellavel. You are absolutely right @luacmartins. If I find it, I will definitely write it down.

Did you edit this file @Santhosh-Sellavel ? https://github.com/Expensify/App/blob/9c50db754bc29b340f7f725059d0cca6771fc61f/src/styles/styles.js#L1682-L1691

because if you add alignItems: 'center' this line looks like that.

If yes, can you please delete that line and try again? According to my proposal it should work like the video below. Thanks.

https://user-images.githubusercontent.com/6093207/160481074-771b9c06-7998-476e-ba3b-57130e65f892.mp4

[Santhosh-Sellavel]

Did you edit this file @Santhosh-Sellavel ?

https://github.com/Expensify/App/blob/9c50db754bc29b340f7f725059d0cca6771fc61f/src/styles/styles.js#L1682-L1691

because if you add alignItems: 'center' this line looks like that.

No, I didn’t @metehanozyurt

[luacmartins]

You are absolutely right @luacmartins. If I find it, I will definitely write it down.

Thanks @metehanozyurt! I think it's worth exploring a css only solution before we introduce more complex logic. Let me know if you come up with anything.

[metehanozyurt]

Thank you @luacmartins and @Santhosh-Sellavel. I think i found the css change we need. Can you please check my solution. Only We need to change like this. Thank you so much for your kind words and encouragements 🙏 .

PDFView: {
        display: 'grid',
        backgroundColor: themeColors.modalBackground,
        width: '100%',
        height: '100%',
        justifyContent: 'center',
        overflow: 'hidden',
        overflowY: 'auto',
        alignItems: 'center',
    },

https://user-images.githubusercontent.com/6093207/161149981-2c54c963-97fa-44c0-8079-2116230bf1d6.mp4

[luacmartins]

@metehanozyurt Nice one! I tested in all platforms and that seemed to fix the issue in all of them! I thought that display: grid wasn't supported in native devices, but that property seems to only affect web/desktop anyways as the solution works on native without display: grid. @Santhosh-Sellavel any comments on the proposed solution?

[Santhosh-Sellavel]

@luacmartins Solution seems to be working! But wondering why display: grid is needed here?

@metehanozyurt how does display: grid addresses the issue?

[metehanozyurt]

Thank you @luacmartins and @Santhosh-Sellavel. My first approach was as follows. I want to implement "justify-content" in small pdf sizes. but I don't need this setting for large pdf sizes. According to the this URL grid offers me this opportunity.

Maybe this URL can be answer about addresses the issue. Like you, I have doubts that it is the only solution.

[Santhosh-Sellavel]

@luacmartins Let's go with the @metehanozyurt proposal here! I don't see any downside here. 🎀 👀 🎀 C+ reviewed

With a couple of suggestions.

• Add display:grid to display utilites
• And use it in [web/desktop implementation] (https://github.com/Expensify/App/blob/main/src/components/PDFView/index.js) alone.

[luacmartins]

Agreed! Your solution looks good @metehanozyurt!

As for @Santhosh-Sellavel suggestions, I would not add display: grid to display utilities though, since it's not supported by RN - https://github.com/Expensify/App/blob/main/src/styles/utilities/display.js#L4-L6.

[MelvinBot]

📣 @metehanozyurt You have been assigned to this job by @luacmartins! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[kevinksullivan]

@metehanozyurt apply to the job when you get a chance. Thanks!

[Santhosh-Sellavel]

As for @Santhosh-Sellavel suggestions, I would not add display: grid to display utilities though, since it's not supported by RN - https://github.com/Expensify/App/blob/main/src/styles/utilities/display.js#L4-L6.

Nice catch @luacmartins , yes adding there could create some unnescessary confusion.

But the reason behind the suggestion is to use display: gridonly for web/desktop.

cc: @metehanozyurt

[luacmartins]

PR was merged. Not overdue.

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.1.55-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/8474

If no regressions arise, payment will be issued on 2022-04-27. :confetti_ball:

[kevinksullivan]

@Santhosh-Sellavel and @thesahindia paid.

@metehanozyurt can you accept the offer so i can finish payment for you? Thanks!

[kevinksullivan]

All set, thanks!