Fix newline breaks in markdown #8673
Conversation
|
Hey @CyberAndrii, thanks for taking the time to investigate this and provide a fix. @lyndaidaii will review this PR and support you in any PR comments that come up. |
|
Looks great to me. Already verified changes. Overall, it improved how existing readme display, made prettier. However, It might impact few existing readme. I think it's great contribution and encourage users to adapt best practice of markdown readme. |
| @@ -77,6 +77,8 @@ public void EncodesHtmlInMarkdownWithAdaptiveHeader(string originalMd, string ex | |||
| [InlineData("\ufeff# Heading with BOM", "<h2>Heading with BOM</h2>", false, false)] | |||
| [InlineData("- List", "<ul>\n<li>List</li>\n</ul>", false, true)] | |||
| [InlineData("- List", "<ul>\r\n<li>List</li>\r\n</ul>", false, false)] | |||
| [InlineData("This is a paragraph\nwithout a break inside", "<p>This is a paragraph\nwithout a break inside</p>", false, true)] | |||
| [InlineData("This is a paragraph\r\nwithout a break inside", "<p>This is a paragraph\r\nwithout a break inside</p>", false, false)] | |||
There was a problem hiding this comment.
A break should still be inserted if I have a readme with multiple empty lines, right? For example:
Hello world
I expect a break between these two lines
Could you add a test for this case?
There was a problem hiding this comment.
EDIT: Please ignore this message as per @lyndaidaii's reply below
@lyndaidaii Should all new markdown tests use markdig? The commonmark code is effectively dead now, right?
| [InlineData("This is a paragraph\r\nwithout a break inside", "<p>This is a paragraph\r\nwithout a break inside</p>", false, false)] | |
| [InlineData("This is a paragraph\r\nwithout a break inside", "<p>This is a paragraph\r\nwithout a break inside</p>", false, true)] |
There was a problem hiding this comment.
We are no longer use commonMark to render markdown file right now. this unit test case here, is because that in commonMark we encode first. that's why it appear \r\n instead for commonMark. it will nice to have a this case included as we did
[InlineData("- List", "<ul>\n<li>List</li>\n</ul>", false, true)]
[InlineData("- List", "<ul>\r\n<li>List</li>\r\n</ul>", false, false)]
Also This is a paragraph\r\nwithout a break inside", "<p>This is a paragraph\r\nwithout a break inside</p>
is same to 'This is a paragraph\nwithout a break inside', only \r\n is encoded in commonMark.
@loic-sharma, let me know if it make sense?
There was a problem hiding this comment.
@CyberAndrii Would you like to add unit test as Loic suggested above? thanks
There was a problem hiding this comment.
Sorry, missed his message.
It converts to separate paragraphs regardless of the number of blank lines (same as on github):
Is this right?
There was a problem hiding this comment.
Thanks for adding this unit test with multiple new line. we will treat as one line here.
I added unit test for soft line break and hard line break. also include your multiple new line cases.
You can create line breaks in two ways.
Soft line break
Hardline break
Soft line breaks can be created by adding two spaces at the end of the line. This way markdown will render each line to be separate lines.
Hardline breaks can be created by inserting an empty line between each line.
Thank you for your contribution!
There was a problem hiding this comment.
@loic-sharma, could you take a look at changes we added for unit test?
Fixes #8642