search

se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
27 stars 411 forks source link

[#98] Space character dropped at line break when converting to PDF #192

Closed Eclipse-Dominator closed 1 year ago

Eclipse-Dominator commented 1 year ago

Fixes #98

In the pdf version of UG, commands that span across more than one line when copied-pasted to the application will drop the line-break. As line-breaks will usually be formatted at spaces when converting to pdf, this will cause the space in command to be dropped when pasting into the application.

Therefore, let's update the UG to inform the users about this behavior.

canihasreview[bot] commented 1 year 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 1 year ago

v1

@Eclipse-Dominator submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/192/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
codecov[bot] commented 1 year ago

Codecov Report

Merging #192 (98173a1) into master (f0e9069) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 98173a1 differs from pull request most recent head 2c3c057. Consider uploading reports for the commit 2c3c057 to get more accurate results

@@            Coverage Diff            @@
##             master     #192   +/-   ##
=========================================
  Coverage     72.06%   72.06%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1235     1235           
  Branches        127      127           
=========================================
  Hits            890      890           
  Misses          314      314           
  Partials         31       31

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

canihasreview[bot] commented 1 year ago

v2

@Eclipse-Dominator submitted v2 for review.

(: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/192/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v3

@Eclipse-Dominator submitted v3 for review.

(: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/192/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

@se-edu/tech-team-level1 for your review ... Remember to review commit-by-commit, and review the commit message as well.

Eclipse-Dominator commented 1 year ago
image

The addition of a newline has resulted in the 'help' command being pushed to a new page in the command table, which unfortunately led to an overlap in the rendering. This issue needs to be addressed before proceeding with the merge.

By the way, using incognito mode might be helpful in debugging ;)

The commit message could be more precise and clearer.

@SPWwj, I'm not sure how to get page like rendering when viewing user guide in browser.

But I wasn't actually able to replicate this when converting to PDF with both Github Pages and local jekyll server. Conversion to pdf tested on:

image

Update

I was able to replicate this issue. It seems to be related to the service that is used to convert the html to pdf. If the option "Save as PDF" is selected, the table rendering will be normal like above.

However, the rendering issue seems to be caused by Microsoft's print to PDF service.

image

SPWwj commented 1 year ago
image

The addition of a newline has resulted in the 'help' command being pushed to a new page in the command table, which unfortunately led to an overlap in the rendering. This issue needs to be addressed before proceeding with the merge. By the way, using incognito mode might be helpful in debugging ;) The commit message could be more precise and clearer.

@SPWwj, I'm not sure how to get page like rendering when viewing user guide in browser.

But I wasn't actually able to replicate this when converting to PDF with both Github Pages and local jekyll server. Conversion to pdf tested on:

  • Edge 114.0.1823.43 (Official build) (64-bit)
  • Chrome 114.0.5735.110 (Official Build) (64-bit)
  • Chrome 114.0.5735.134 (Official Build) (64-bit)

image

image

Window:

My bad, since the recommended setting is A4, this is not a issue...

SPWwj commented 1 year ago
  • Update ug regarding copying commands

Just a minor suggestion for the commit message: "Update UG with clarified instructions on copying commands". While the current commit message is effective, someone without any prior knowledge of the issue might question what 'regarding copying commands' refers to, as it is rather vague. Therefore, if it fits within the character limit, we should aim to enhance its conciseness.

Other than that this PR look good to me :)

canihasreview[bot] commented 1 year ago

v4

@Eclipse-Dominator submitted v4 for review.

(: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/192/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

Merged with minor changes to the commit message (revised the subject, removed 'Therefore,').

Thanks for the PR, @Eclipse-Dominator and for the reviews @SPWwj @jundatan @dlimyy