RuiFengg commented 3 years ago

Fixes #59

The page breaks cut through content if no specific media query is specified

Let's fix this by applying the media print query break-inside: avoid on body elements

I've tested this on my 2103 team's UG and on AB3 to verify the fix

canihasreview[bot] commented 3 years ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

canihasreview[bot] commented 3 years ago

v1

@RuiFengg submitted v1 for review.

[v1 1/1] Fix page break issue

(:books: Archive)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/76/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

damithc commented 3 years ago

Thanks for the PR @RuiFengg Let's wait to see if anyone can review it.

wxwxwxwx9 commented 3 years ago

Hi there! I took a quick look at the problem and the proposed solution. It seems like the page-break problem stems from the use of display: flex. Thus, in the commit, the issue was actually solved by replacing display: flex (flexbox) with display: block. According to what I found out, it seems like flexbox doesn't go well with page-break in browsers generally.

If you were to remove break-inside: avoid and keep only display: block, I think the problem would still be fixed. I tried it locally and it seems to be the case. Perhaps you can verify it on your side as well?

I think a possible hotfix can be adding a print media query:

@media print {
    body {
        display: block;
    }
}

It would be great if you can help me test out whether the hotfix would resolve all the buggy cases that you have encountered so far :-)

Regarding a proper fix -- I'm not sure why display: flex was used in the first place. But there might have a been a good reason for it; thus I'm not sure if it would be good to just change it to display: block.

If we can ascertain that changing display: flex to display: block would not cause any regressions, then I think that would be the way to go forward.

RuiFengg commented 3 years ago

@wxwxwxwx9 Thx for helping me check through! I tried the hotfix suggested on my end and it does solve the page break issue as well. Should we go ahead with this?

damithc commented 3 years ago

@wxwxwxwx9 Thx for helping me check through! I tried the hotfix suggested on my end and it does solve the page break issue as well. Should we go ahead with this?

Yes, let's try that.

RuiFengg commented 3 years ago

@wxwxwxwx9 Thx for helping me check through! I tried the hotfix suggested on my end and it does solve the page break issue as well. Should we go ahead with this?

Yes, let's try that.

Alright I've updated the PR

codecov-io commented 3 years ago

Codecov Report

Merging #76 (17f455c) into master (1fe37e1) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #76   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1fe37e1...17f455c. Read the comment docs.

damithc commented 3 years ago

Alright I've updated the PR

@RuiFengg use the same bot you used earlier to submit a new version.

canihasreview[bot] commented 3 years ago

v2

@RuiFengg submitted v2 for review.

[v2 1/2] Fix page break issue
[v2 2/2] Update page break fix

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/76/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

damithc commented 3 years ago

@RuiFengg there should only be one commit as this is just one change?

canihasreview[bot] commented 3 years ago

v3

@RuiFengg submitted v3 for review.

[v3 1/1] Fix page break issue

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/76/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

RuiFengg commented 3 years ago

@RuiFengg there should only be one commit as this is just one change?

Oops sry about that. I've updated the PR

damithc commented 3 years ago

@se-edu/tech-team-level1I would like reviews from some of you for this PR please

Zhiyuan-Amos commented 3 years ago

@se-edu/tech-team-level1I would like reviews from some of you for this PR please

@damithc the @se-edu/tech-team-level1 tag in the message that you sent above did not go through because it's missing a whitespace between 1 and I :P

@se-edu/tech-team-level1 tagging on prof's behalf

siangernlow commented 3 years ago

Hi, I've tested the solution on my group's DG and it has worked fine as well. Some suggestions below.

It might be suitable to add a comment above the media print query to document its purpose. For example, a comment shown below might be useful.
```
/**
* Prevents page breaks when converting to PDF
* /
```
Adding css media print queries will allow the pages to be printed out without text lines being cut

Perhaps it would be better to describe the solution as Adding a css media print query instead since there was only a single query added in the commit.

Refer to this S/0 discussion on dealing with page breaks https://stackoverflow.com/questions/907680/css-printing-avoiding-cut-in-half-divs-between-pages

The S/O discussion is not as relevant now since the solution has changed. Perhaps it would be more suitable to explain why it is done that way, i.e. the fact that using a media print query prevents the styles of other media types from being affected.

damithc commented 3 years ago

thanks for the comments @siangernlow @RuiFengg what do you think (of the comments)?

canihasreview[bot] commented 3 years ago

v4

@RuiFengg submitted v4 for review.

[v4 1/1] Fix page break issue

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/76/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

RuiFengg commented 3 years ago

thanks for the comments @siangernlow @RuiFengg what do you think (of the comments)?

Thanks for reviewing @siangernlow @damithc The comments were all relevant and the changes have been applied respectively. Edited the suggested code comment to be a bit more specific as well

damithc commented 3 years ago

@wxwxwxwx9 can provide a review to this PR?

wxwxwxwx9 commented 3 years ago

@damithc Hi prof, the changes look good to me! :-)

damithc commented 3 years ago

@RuiFengg would the following commit message work? i.e., is it accurate and contain all the important information? I shortened yours, added full stops, and used the full width (72 characters)

Fix page breaks cutting through content  when converted to PDF (#76)

Converting documents to PDF causes page breaks to cut through content.

As the PDF conversion is achieved by printing the Web page to a PDF,
let's specify the behaviour of the media print query to change the
display type of the 'body' component to 'block', which fixes this issue.

RuiFengg commented 3 years ago

@RuiFengg would the following commit message work? i.e., is it accurate and contain all the important information? I shortened yours, added full stops, and used the full width (72 characters)
Fix page breaks cutting through content  when converted to PDF (#76)

Converting documents to PDF causes page breaks to cut through content.

As the PDF conversion is achieved by printing the Web page to a PDF,
let's specify the behaviour of the media print query to change the
display type of the 'body' component to 'block', which fixes this issue.

Yup this summarised version does contain all the important information describing and explaining the fix. Thanks for the edit! I will update the commit message

canihasreview[bot] commented 3 years ago

v5

@RuiFengg submitted v5 for review.

[v5 1/1] Fix page breaks cutting through content when converted to PDF (#76)

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v4 and v5) (:chart_with_upwards_trend: Range-Diff between v4 and v5)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/76/5/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

damithc commented 3 years ago

Thanks @RuiFengg for the PR. I merged with a minor edit to the commit subject. Thanks for your inputs @wxwxwxwx9 @siangernlow

se-edu / addressbook-level3

Page breaks cut through content when converted to PDF #59 #76

v1

Codecov Report

v2

v3

v4

v5