-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#98] Space character dropped at line break when converting to PDF #192
[#98] Space character dropped at line break when converting to PDF #192
Conversation
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
v1@Eclipse-Dominator submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/192/1/head:BRANCHNAME where |
Codecov Report
@@ 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 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM! Just some suggestions below
docs/UserGuide.md
Outdated
@@ -63,6 +63,7 @@ AddressBook Level 3 (AB3) is a **desktop app for managing contacts, optimized fo | |||
* Extraneous parameters for commands that do not take in parameters (such as `help`, `list`, `exit` and `clear`) will be ignored.<br> | |||
e.g. if the command specifies `help 123`, it will be interpreted as `help`. | |||
|
|||
* Be careful when copy-pasting commands that cut across a single line from this user guide as `Sample commands` that span more than a single line may drop a space at the line-break when copied over to the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can consider shortening Sample commands ...
to these commands
to make it less verbose
* Be careful when copy-pasting commands that cut across a single line from this user guide as `Sample commands` that span more than a single line may drop a space at the line-break when copied over to the application. | |
* Be careful when copy-pasting commands that span more than a single line from this user guide as the space character at the line-break may be lost when these commands are copied over to the application. |
docs/UserGuide.md
Outdated
@@ -63,6 +63,7 @@ AddressBook Level 3 (AB3) is a **desktop app for managing contacts, optimized fo | |||
* Extraneous parameters for commands that do not take in parameters (such as `help`, `list`, `exit` and `clear`) will be ignored.<br> | |||
e.g. if the command specifies `help 123`, it will be interpreted as `help`. | |||
|
|||
* Be careful when copy-pasting commands that cut across a single line from this user guide as `Sample commands` that span more than a single line may drop a space at the line-break when copied over to the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative that mentions PDF files specifically.
* Be careful when copy-pasting commands that cut across a single line from this user guide as `Sample commands` that span more than a single line may drop a space at the line-break when copied over to the application. | |
* If you are using a PDF version of this document, be careful when copy-pasting commands that span multiple lines, as spaces surrounding line breaks can get omitted when copy-pasted from a PDF file. |
dd247f6
to
a8834be
Compare
v2@Eclipse-Dominator submitted v2 for review.
(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/192/2/head:BRANCHNAME where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit message:
*span across more than one line -> span multiple lines
- Try using ChatGPT to see if the commit message phrasing can be improved.
UserGuide.md: there is a typo
v3@Eclipse-Dominator submitted v3 for review.
(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/192/3/head:BRANCHNAME where |
a8834be
to
98173a1
Compare
@se-edu/tech-team-level1 for your review ... |
docs/UserGuide.md
Outdated
@@ -63,6 +63,7 @@ AddressBook Level 3 (AB3) is a **desktop app for managing contacts, optimized fo | |||
* Extraneous parameters for commands that do not take in parameters (such as `help`, `list`, `exit` and `clear`) will be ignored.<br> | |||
e.g. if the command specifies `help 123`, it will be interpreted as `help`. | |||
|
|||
* If you are using a PDF version of this document, be careful when copy-pasting commands that span multiple lines as space characters surrounding line-breaks may be omitted when copied over to the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo, is quite odd to put this under the "notes about the command format" section as it does not fit well with the other pointers in it. Perhaps, it will be better to be put it in a separate section like the FAQ or below the features header outside of the "notes about the command format" section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, positioning this information here is beneficial as it pertains directly to the use of commands. From personal experience, I don't typically read through the entire User Guide before starting to use the commands. Therefore, having notes about potential pitfalls or issues within the command format section can be extremely useful.
Furthermore, I view the FAQ as an additional, somewhat less critical feature, which should not interfere with or impede the fundamental understanding and usage of the commands. As such, I believe it's acceptable to prioritize the 'Note about the command' section over the FAQ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for inputs @jundatan @SPWwj
Yes it is a bit odd to have in under 'notes about the command format'. Just below it as a separate box is one viable option. Putting it in the FAQ is not a good idea, as @SPWwj mentioned. For now, let's keep it in the current place as it is less disruptive to the flow of the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. @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.
UpdateI was able to replicate this issue. It seems to be related to the service that is used to convert the html to pdf. However, the rendering issue seems to be caused by Microsoft's print to PDF service. |
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. Window:
My bad, since the recommended setting is A4, this is not a issue... |
docs/UserGuide.md
Outdated
@@ -63,6 +63,7 @@ AddressBook Level 3 (AB3) is a **desktop app for managing contacts, optimized fo | |||
* Extraneous parameters for commands that do not take in parameters (such as `help`, `list`, `exit` and `clear`) will be ignored.<br> | |||
e.g. if the command specifies `help 123`, it will be interpreted as `help`. | |||
|
|||
* If you are using a PDF version of this document, be careful when copy-pasting commands that span multiple lines as space characters surrounding line-breaks may be omitted when copied over to the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor alteration suggestion:
It may be less confusing to users if we adjust the wording regarding the copy-pasting commands. Instead of suggesting that AB3 has a copy-paste command, we could articulate it as 'copying and pasting the command from the PDF version of the User Guide.'
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 :) |
In the PDF version of the UG, commands that span multiple lines lose their line breaks when copied and pasted into the application. Since line breaks are typically converted to spaces when the document is converted to PDF, this results in the omission of spaces within the command when pasted into the application. Therefore, let's update the UG to inform users about this behavior.
98173a1
to
2c3c057
Compare
v4@Eclipse-Dominator submitted v4 for review. (📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/192/4/head:BRANCHNAME where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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 |
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.