Skip to content
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.

Sign up for GitHub

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

Fix newline breaks in markdown #8673

Merged
merged 4 commits into from
Jul 19, 2021
Merged

Fix newline breaks in markdown #8673

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8402516
Fix newline breaks in markdown
CyberAndrii Jul 9, 2021
04a6567
Add test with multiple empty lines
CyberAndrii Jul 19, 2021
3784c8b
include soft&hard line break unit test
Jul 19, 2021
b2ba6d2
remove unit
Jul 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/NuGetGallery/Services/MarkdownService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ private RenderedMarkdownResult GetHtmlFromMarkdownCommonMark(string markdownStri
var encodedMarkdown = EncodedBlockQuotePattern.Replace(HttpUtility.HtmlEncode(readmeWithoutBom), "> ");

var settings = CommonMarkSettings.Default.Clone();
settings.RenderSoftLineBreaksAsLineBreaks = true;

// Parse executes CommonMarkConverter's ProcessStage1 and ProcessStage2.
var document = CommonMarkConverter.Parse(encodedMarkdown, settings);
Expand Down Expand Up @@ -196,7 +195,6 @@ private RenderedMarkdownResult GetHtmlFromMarkdownMarkdig(string markdownString,
.UsePipeTables()
.UseListExtras()
.UseTaskLists()
.UseSoftlineBreakAsHardlineBreak()
.UseEmojiAndSmiley()
.UseAutoLinks()
.UseReferralLinks("noopener noreferrer nofollow")
Expand Down
13 changes: 2 additions & 11 deletions tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@loic-sharma loic-sharma Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Suggested change
[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)]

Copy link
Contributor

@lyndaidaii lyndaidaii Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@CyberAndrii Would you like to add unit test as Loic suggested above? thanks

Copy link
Contributor Author

@CyberAndrii CyberAndrii Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, missed his message.

It converts to separate paragraphs regardless of the number of blank lines (same as on github):

image

image

Is this right?

Copy link
Contributor

@lyndaidaii lyndaidaii Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

@loic-sharma, could you take a look at changes we added for unit test?

[InlineData("[text](http://www.test.com)", "<p><a href=\"http://www.test.com/\" rel=\"noopener noreferrer nofollow\">text</a></p>", false, true)]
[InlineData("[text](http://www.test.com)", "<p><a href=\"http://www.test.com/\" rel=\"noopener noreferrer nofollow\">text</a></p>", false, false)]
[InlineData("[text](javascript:alert('hi'))", "<p><a href=\"\" rel=\"noopener noreferrer nofollow\">text</a></p>", false, true)]
Expand Down Expand Up @@ -141,17 +143,6 @@ public void ConvertsMarkdownToHtmlWithoutImageDisaplyed(bool isMarkdigMdRenderin
Assert.True(readMeResult.ImageSourceDisallowed);
}

[Fact]
public void TestToHtmlWithExtension()
{
var originalMd = "This is a paragraph\r\n with a break inside";
var expectedHtml = "<p>This is a paragraph<br />\nwith a break inside</p>";
_featureFlagService.Setup(x => x.IsMarkdigMdRenderingEnabled()).Returns(true);
var readMeResult = _markdownService.GetHtmlFromMarkdown(originalMd);
Assert.Equal(expectedHtml, readMeResult.Content);
Assert.False(readMeResult.ImagesRewritten);
}

[Fact]
public void TestToHtmlWithPipeTable()
{
Expand Down