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

mvtglobally · 2022-03-14T04:52:41Z

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:

Go to Chat
Try sending the pdf given in thread
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 · 2022-03-14T04:52:44Z

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

MelvinBot · 2022-03-14T20:29:18Z

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

MelvinBot · 2022-03-14T20:29:21Z

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

kevinksullivan · 2022-03-14T20:29:49Z

@thesahindia please apply when you get a chance!

kevinksullivan · 2022-03-22T20:57:00Z

Price doubled.

metehanozyurt · 2022-03-27T22:35:16Z

Proposal

Cause

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

Solution

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});  
    }  
}

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});  
}

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',
} : {};

We should pass the calculated style to the View:

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

20220328_013327.mp4

luacmartins · 2022-03-28T18:38:04Z

@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 · 2022-03-28T19:07:46Z

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

@metehanozyurt Please add an updated proposal address this one #8127 (comment)

metehanozyurt · 2022-03-28T21:23:46Z

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;

App/src/styles/styles.js

Lines 1682 to 1691 in 9c50db7

    
           PDFView: { 
        
               flex: 1, 
        
               backgroundColor: themeColors.modalBackground, 
        
               width: '100%', 
        
               height: '100%', 
        
               flexDirection: 'row', 
        
               justifyContent: 'center', 
        
               overflow: 'hidden', 
        
               overflowY: 'auto', 
        
           },

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

20220328_231652.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:

20220328_231812.mp4

kevinksullivan · 2022-03-29T20:49:59Z

Proposal in review

Santhosh-Sellavel · 2022-03-30T17:27:11Z

Reviewing this 👀

Santhosh-Sellavel · 2022-03-30T18:06:24Z

@metehanozyurt is this your updated proposal?

Santhosh-Sellavel · 2022-03-30T18:08:55Z

@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

luacmartins · 2022-03-30T18:43:19Z

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 · 2022-03-30T18:45:58Z

Yep would be nice & clean!

metehanozyurt · 2022-03-31T05:50:29Z

@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 ?

App/src/styles/styles.js

Lines 1682 to 1691 in 9c50db7

    
           PDFView: { 
        
               flex: 1, 
        
               backgroundColor: themeColors.modalBackground, 
        
               width: '100%', 
        
               height: '100%', 
        
               flexDirection: 'row', 
        
               justifyContent: 'center', 
        
               overflow: 'hidden', 
        
               overflowY: 'auto', 
        
           },

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.

20220328_231812.mp4

Santhosh-Sellavel · 2022-03-31T13:58:30Z

Did you edit this file @Santhosh-Sellavel ?

App/src/styles/styles.js

Lines 1682 to 1691 in 9c50db7

PDFView: {

flex: 1,

backgroundColor: themeColors.modalBackground,

width: '100%',

height: '100%',

flexDirection: 'row',

justifyContent: 'center',

overflow: 'hidden',

overflowY: 'auto',

},

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

No, I didn’t @metehanozyurt

luacmartins · 2022-03-31T15:25:56Z

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 · 2022-03-31T21:01:30Z

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',
    },

20220401_000954.mp4

luacmartins · 2022-04-01T15:40:02Z

@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 · 2022-04-01T16:20:32Z

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

@metehanozyurt how does display: grid addresses the issue?

metehanozyurt · 2022-04-01T19:16:29Z

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 · 2022-04-01T19:39:01Z

@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 · 2022-04-01T20:46:01Z

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 · 2022-04-01T20:46:11Z

📣 @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 · 2022-04-01T23:10:54Z

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

Santhosh-Sellavel · 2022-04-02T03:31:20Z

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 · 2022-04-11T18:23:23Z

PR was merged. Not overdue.

melvin-bot · 2022-04-20T18:29:23Z

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

Fix: Pdf preview is not in centered #8474

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

kevinksullivan · 2022-04-28T03:11:26Z

@Santhosh-Sellavel and @thesahindia paid.

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

kevinksullivan · 2022-04-28T17:33:08Z

All set, thanks!

mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Mar 14, 2022

MelvinBot assigned kevinksullivan Mar 14, 2022

MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 14, 2022

kevinksullivan added the Exported label Mar 14, 2022

botify removed the Daily KSv2 label Mar 14, 2022

MelvinBot added the Weekly KSv2 label Mar 14, 2022

MelvinBot assigned Santhosh-Sellavel Mar 14, 2022

MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 14, 2022

MelvinBot assigned luacmartins Mar 14, 2022

kevinksullivan changed the title ~~Pdf preview is not in centered - reported by @thesahindia~~ [$500] Pdf preview is not in centered - reported by @thesahindia Mar 22, 2022

MelvinBot added the Overdue label Mar 22, 2022

MelvinBot removed the Overdue label Mar 22, 2022

luacmartins assigned metehanozyurt Apr 1, 2022

MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 1, 2022

This was referenced Apr 2, 2022

issue:8127.Pdf preview center. Add display style #8450

Closed

Fix: Pdf preview is not in centered #8474

Merged

melvin-bot bot added the Overdue label Apr 11, 2022

luacmartins added the Reviewing Has a PR in review label Apr 11, 2022

melvin-bot bot removed the Overdue label Apr 11, 2022

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 20, 2022

melvin-bot bot changed the title ~~[$500] Pdf preview is not in centered - reported by @thesahindia~~ [HOLD for payment 2022-04-27] [$500] Pdf preview is not in centered - reported by @thesahindia Apr 20, 2022

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 26, 2022

kevinksullivan closed this as completed Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mvtglobally commented Mar 14, 2022 •

edited by kevinksullivan

Loading

MelvinBot commented Mar 14, 2022

MelvinBot commented Mar 14, 2022

MelvinBot commented Mar 14, 2022

kevinksullivan commented Mar 14, 2022

kevinksullivan commented Mar 22, 2022

metehanozyurt commented Mar 27, 2022 •

edited

Loading

luacmartins commented Mar 28, 2022

Santhosh-Sellavel commented Mar 28, 2022

metehanozyurt commented Mar 28, 2022

kevinksullivan commented Mar 29, 2022

Santhosh-Sellavel commented Mar 30, 2022

Santhosh-Sellavel commented Mar 30, 2022

Santhosh-Sellavel commented Mar 30, 2022 •

edited

Loading

luacmartins commented Mar 30, 2022

Santhosh-Sellavel commented Mar 30, 2022

metehanozyurt commented Mar 31, 2022 •

edited

Loading

Santhosh-Sellavel commented Mar 31, 2022 •

edited

Loading

luacmartins commented Mar 31, 2022

metehanozyurt commented Mar 31, 2022 •

edited

Loading

luacmartins commented Apr 1, 2022

Santhosh-Sellavel commented Apr 1, 2022

metehanozyurt commented Apr 1, 2022

Santhosh-Sellavel commented Apr 1, 2022 •

edited

Loading

luacmartins commented Apr 1, 2022

MelvinBot commented Apr 1, 2022

kevinksullivan commented Apr 1, 2022

Santhosh-Sellavel commented Apr 2, 2022

luacmartins commented Apr 11, 2022

melvin-bot bot commented Apr 20, 2022

kevinksullivan commented Apr 28, 2022

kevinksullivan commented Apr 28, 2022

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

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

Comments

mvtglobally commented Mar 14, 2022 • edited by kevinksullivan Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

MelvinBot commented Mar 14, 2022

MelvinBot commented Mar 14, 2022

MelvinBot commented Mar 14, 2022

kevinksullivan commented Mar 14, 2022

kevinksullivan commented Mar 22, 2022

metehanozyurt commented Mar 27, 2022 • edited Loading

Proposal

Cause

Solution

luacmartins commented Mar 28, 2022

Santhosh-Sellavel commented Mar 28, 2022

metehanozyurt commented Mar 28, 2022

kevinksullivan commented Mar 29, 2022

Santhosh-Sellavel commented Mar 30, 2022

Santhosh-Sellavel commented Mar 30, 2022

Santhosh-Sellavel commented Mar 30, 2022 • edited Loading

luacmartins commented Mar 30, 2022

Santhosh-Sellavel commented Mar 30, 2022

metehanozyurt commented Mar 31, 2022 • edited Loading

Santhosh-Sellavel commented Mar 31, 2022 • edited Loading

luacmartins commented Mar 31, 2022

metehanozyurt commented Mar 31, 2022 • edited Loading

luacmartins commented Apr 1, 2022

Santhosh-Sellavel commented Apr 1, 2022

metehanozyurt commented Apr 1, 2022

Santhosh-Sellavel commented Apr 1, 2022 • edited Loading

luacmartins commented Apr 1, 2022

MelvinBot commented Apr 1, 2022

kevinksullivan commented Apr 1, 2022

Santhosh-Sellavel commented Apr 2, 2022

luacmartins commented Apr 11, 2022

melvin-bot bot commented Apr 20, 2022

kevinksullivan commented Apr 28, 2022

kevinksullivan commented Apr 28, 2022

mvtglobally commented Mar 14, 2022 •

edited by kevinksullivan

Loading

metehanozyurt commented Mar 27, 2022 •

edited

Loading

Santhosh-Sellavel commented Mar 30, 2022 •

edited

Loading

metehanozyurt commented Mar 31, 2022 •

edited

Loading

Santhosh-Sellavel commented Mar 31, 2022 •

edited

Loading

metehanozyurt commented Mar 31, 2022 •

edited

Loading

Santhosh-Sellavel commented Apr 1, 2022 •

edited

Loading