Skip to content

Add line-numbers when wrapping is needed for printing#2431

Merged
yucheng11122017 merged 18 commits into
MarkBind:masterfrom
Tim-Siu:soft-wrap-print
Feb 28, 2024
Merged

Add line-numbers when wrapping is needed for printing#2431
yucheng11122017 merged 18 commits into
MarkBind:masterfrom
Tim-Siu:soft-wrap-print

Conversation

@Tim-Siu

@Tim-Siu Tim-Siu commented Feb 23, 2024

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Make code wrapping the default behavior for printing. Add line numbers when wrapping does happen.
Related PR: #2413
Related Issue: #2404

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Automatically enforce soft-wrapping and add line-numbers when code is cut off

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@codecov

codecov Bot commented Feb 23, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.92%. Comparing base (e1ba80e) to head (cd664e8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2431   +/-   ##
=======================================
  Coverage   48.92%   48.92%           
=======================================
  Files         124      124           
  Lines        5245     5245           
  Branches     1110     1110           
=======================================
  Hits         2566     2566           
  Misses       2371     2371           
  Partials      308      308           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EltonGohJH

EltonGohJH commented Feb 23, 2024

Copy link
Copy Markdown
Contributor

@Tim-Siu
It generally looks good to me. Thanks for implementing this.
There is a concern on my end tho.

It seems like once you trigger printing then the CSS remains thus the line numbers. In my opinion, it is not easy to undo it. (Prob hacky to undo it). What do you think? If it is too hard and not worthwhile, maybe can ask damith what he thinks about this side effect.

Do add documentation. (Also this side effect if we are going with this)

@Tim-Siu

Tim-Siu commented Feb 23, 2024

Copy link
Copy Markdown
Contributor Author

@Tim-Siu It generally looks good to me. Thanks for implementing this. There is a concern on my end tho.

It seems like once you trigger printing then the CSS remains thus the line numbers. In my opinion, it is not easy to undo it. (Prob hacky to undo it). What do you think? If it is too hard and not worthwhile, maybe can ask damith what he thinks about this side effect.

Do add documentation. (Also this side effect if we are going with this)

Sorry I overlooked the bug. It should be fine now. I will write the documentation for this new property right now.

@Tim-Siu

Tim-Siu commented Feb 23, 2024

Copy link
Copy Markdown
Contributor Author

Currently, we don't have existing documentation for printing optimisation. Should we add such a section under reference?

@EltonGohJH

Copy link
Copy Markdown
Contributor

@Tim-Siu
Do add that. Do add that in the code block section too.

@Tim-Siu

Tim-Siu commented Feb 24, 2024

Copy link
Copy Markdown
Contributor Author

I have added a printing optimization section under the code. I am not too sure about adding a dedicated section about Printing Optimisation under reference though. Most optimizations we do seem quite natural and are quite unlikely to cause too much surprise.

I will raise a dedicated PR to add a Printing Optimisation section about all the optimizations if it is considered useful and appropriate.

yucheng11122017 and others added 5 commits February 24, 2024 23:16
* enable markdown formating for content in annotate point

* add slot for header and label in annotation point

* add documentation on using markdown in content label and header

* add functional test

* add test cases for annotation
There is no checking of proposed commit message presence.

PR authors may forget to include proposed commit message.

Adding a check using GitHub Actions will help remind and ensure
that authors don't miss out on filling in the commit message.

Let's add a job to the a new workflow, pr-message-reminder.yml
file to help automate checking and reminding of filling in the
proposed commit message for each PR.

This approach automates the process, without having to have
other users check and remind PR authors themselves. Adding the
new job to a new workflow will allow greater control of job triggers
while maintaining clean code.
@Tim-Siu Tim-Siu marked this pull request as draft February 24, 2024 15:25
@Tim-Siu Tim-Siu marked this pull request as ready for review February 24, 2024 15:28
@yucheng11122017

yucheng11122017 commented Feb 25, 2024

Copy link
Copy Markdown
Contributor

I have added a printing optimization section under the code. I am not too sure about adding a dedicated section about Printing Optimisation under reference though. Most optimizations we do seem quite natural and are quite unlikely to cause too much surprise.

I will raise a dedicated PR to add a Printing Optimisation section about all the optimizations if it is considered useful and appropriate.

Hi @Tim-Siu, I think there is already an section dedicated to printing in the user guide! Although it is very small...
Could you just add what you wrote to this part?

@Tim-Siu

Tim-Siu commented Feb 25, 2024

Copy link
Copy Markdown
Contributor Author

Sure! I have added a brief overview of other printing optimisations at that section.

@yucheng11122017

Copy link
Copy Markdown
Contributor

Hi @Tim-Siu thanks for the work so far!

One thing I noticed was when I tried to print, there seems to be a new line

Example:
Code

<tag>
    <diafsdfjnsdkjfnsdkjfskjdfnksjdfnkdfjsdkjfnsdjfksdfjksndfkjsndkfjnsdkjfnksjdnfkjsndfkjsndfknsdjkfnksjdfnkjsdnfjksdfknsdfkjsndf>
    </diafsdfjnsdkjfnsdkjfskjdfnksjdfnkdfjsdkjfnsdjfksdfjksndfkjsndkfjnsdkjfnksjdnfkjsndfkjsndfknsdjkfnksjdfnkjsdnfjksdfknsdfkjsndf>
</tag>

Output:
image

Could you take a look to see what is causing the issue and see if there is a fix for it?

@Tim-Siu

Tim-Siu commented Feb 25, 2024

Copy link
Copy Markdown
Contributor Author

Hi @Tim-Siu thanks for the work so far!

One thing I noticed was when I tried to print, there seems to be a new line

Example: Code

<tag>
    <diafsdfjnsdkjfnsdkjfskjdfnksjdfnkdfjsdkjfnsdjfksdfjksndfkjsndkfjnsdkjfnksjdnfkjsndfkjsndfknsdjkfnksjdfnkjsdnfjksdfknsdfkjsndf>
    </diafsdfjnsdkjfnsdkjfskjdfnksjdfnkdfjsdkjfnsdjfksdfjksndfkjsndkfjnsdkjfnksjdnfkjsndfkjsndfknsdjkfnksjdfnkjsdnfjksdfknsdfkjsndf>
</tag>

Output: image

Could you take a look to see what is causing the issue and see if there is a fix for it?

That's a great catch! Sorry for the overlook. I will look into this.

@Tim-Siu

Tim-Siu commented Feb 25, 2024

Copy link
Copy Markdown
Contributor Author

Hi @Tim-Siu thanks for the work so far!

One thing I noticed was when I tried to print, there seems to be a new line

Example: Code

<tag>
    <diafsdfjnsdkjfnsdkjfskjdfnksjdfnkdfjsdkjfnsdjfksdfjksndfkjsndkfjnsdkjfnksjdnfkjsndfkjsndfknsdjkfnksjdfnkjsdnfjksdfknsdfkjsndf>
    </diafsdfjnsdkjfnsdkjfskjdfnksjdfnkdfjsdkjfnsdjfksdfjksndfkjsndkfjnsdkjfnksjdnfkjsndfkjsndfknsdjkfnksjdfnkjsdnfjksdfknsdfkjsndf>
</tag>

Output: image

Could you take a look to see what is causing the issue and see if there is a fix for it?

HI @yucheng11122017 ,

It should be good now. Thank you for pointing this out!
Screenshot 2024-02-25 at 15 38 13

Comment thread docs/userGuide/syntax/code.md Outdated
##### Printing optimization

Markbind automatically enhances the readability of your code blocks when printing:
- **Light theme for clarity**: Ensures optimal contrast by switching code blocks to the light theme when printing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Tim-Siu, where did you see this? Because the printing still seems to be in dark theme if the code is in dark theme
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-02-25 at 17 29 31 Hi @yucheng11122017 It does work like that on my side. But indeed this seems to be undefined behaviors. I will look into this. I will set the PR to draft for now until I thoroughly understand the CSS for the site. Sorry for the overlook.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi no worries @Tim-Siu! I think for now, this PR can just focus on adding the line numbers, which is what you already did. You can just clean up the documentation to not include these undefined behaviors first, add an issue, and then fix it in the future if you are interested!

@Tim-Siu Tim-Siu marked this pull request as draft February 25, 2024 10:09
@Tim-Siu

Tim-Siu commented Feb 25, 2024

Copy link
Copy Markdown
Contributor Author

HI @yucheng11122017 ,

The code block should now be of a light theme. However, I realized that line highlighting (on both the master and this branch) doesn't seem to work. I am not sure if this is the problem of my system or the code.

height: 100%;
}

/* CSS from codeblock-light.min.css */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than copying the CSS, it might be better to add the class name to the code block? What do you think

@Tim-Siu

Tim-Siu commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Hi @yucheng11122017 ,
Thank you for your advice. I have cleaned up the PR of unrelated changes. I will investigate other printing-related issues and raise issues in separate issues.

@Tim-Siu Tim-Siu marked this pull request as ready for review February 26, 2024 05:46
Comment thread docs/userGuide/syntax/code.md
Comment thread docs/userGuide/reusingContents.md
Utilise content reuse for the documentation

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
@yucheng11122017

Copy link
Copy Markdown
Contributor

Hi @Tim-Siu i think there is some issues with the relative file path! sorry i didn't try it out - i just wrote out the idea

@Tim-Siu

Tim-Siu commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Hi @Tim-Siu i think there is some issues with the relative file path! sorry i didn't try it out - i just wrote out the idea

Understood! It should be working now. Thank you for your patience.

@yucheng11122017

Copy link
Copy Markdown
Contributor

@Tim-Siu hmm the check still seems to be failing

@Tim-Siu

Tim-Siu commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

@Tim-Siu hmm the check still seems to be failing

Hi @yucheng11122017

The Netify test seems to be passing on my side. Are there any additional tests that I have overlooked?

@yucheng11122017 yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I think I saw the old check run!

LGTM thank you for the work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants