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

Add support for markdown > alerts via markdig #10173

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Add support for markdown > alerts via markdig #10173

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af6daf5
Add support for markdown > alerts via markdig
kzu Sep 5, 2024
5cd5170
Make sure alerts use default font color
kzu Sep 5, 2024
3bf38d8
update css style
Oct 21, 2024
0bdf47c
add unit test
Oct 22, 2024
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: 1 addition & 1 deletion Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<PackageVersion Include="LibGit2Sharp" Version="0.26.0" />
<PackageVersion Include="Lucene.Net.Contrib" Version="3.0.3" />
<PackageVersion Include="Lucene.Net" Version="3.0.3" />
<PackageVersion Include="Markdig.Signed" Version="0.30.2" />
<PackageVersion Include="Markdig.Signed" Version="0.37.0" />
<PackageVersion Include="MicroBuild.Core" Version="0.3.0" />
<PackageVersion Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.21.0" />
<PackageVersion Include="Microsoft.ApplicationInsights.TraceListener" Version="2.21.0" />
Expand Down
2 changes: 1 addition & 1 deletion src/Bootstrap/dist/css/bootstrap-theme.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions src/Bootstrap/dist/css/bootstrap.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Bootstrap/less/alerts.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@
.alert-variant(@alert-danger-bg; @alert-danger-border; @alert-danger-text);
}

.alert-primary {
.alert-variant(@alert-primary-bg; @alert-primary-border; @alert-primary-text);
}

.alert-brand-a {
a {
Expand Down
3 changes: 3 additions & 0 deletions src/Bootstrap/less/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,9 @@
@alert-danger-text: #000;
@alert-danger-border: #fde7e9;

@alert-primary-bg: #EFD9FD;
@alert-primary-text: #000;
@alert-primary-border: #EFD9FD;

//== Progress bars
//
Expand Down
2 changes: 1 addition & 1 deletion src/NuGetGallery/Content/gallery/css/bootstrap.min.css
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions src/NuGetGallery/Content/gallery/css/fabric.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
/*
Your use of the content in the files referenced here is subject to the terms of the license at http://aka.ms/fabric-assets-license
*/
.markdown-alert > p {
color: unset !important;
}

@keyframes ms-motion-fadeIn {
from {
opacity: 0;
Expand Down
5 changes: 3 additions & 2 deletions src/NuGetGallery/Services/MarkdownService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -208,6 +208,7 @@ private RenderedMarkdownResult GetHtmlFromMarkdownMarkdig(string markdownString,
.UseTaskLists()
.UseEmojiAndSmiley()
.UseAutoLinks()
.UseAlertBlocks()
Copy link
Member

Choose a reason for hiding this comment

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

I think it's important for the markdown dialect to use with NuGet readmes should be designed with intent, rather than ad-hoc as customers want features.

NuGet.Client is in the process of adding readme support to Visual Studio's Package Manager UI:

If nuget.org supports markdown features that VS does not, that won't be good for customers, either package consumers or package authors.

cc @jgonz120

Copy link
Author

Choose a reason for hiding this comment

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

How does that align with the readme support already there in GitHub? The alert format and markup is fairly standard at this point, I'd say...

Copy link
Member

Choose a reason for hiding this comment

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

from: https://github.github.com/gfm/

GitHub Flavored Markdown, often shortened as GFM, is the dialect of Markdown that is currently supported for user content on GitHub.com and GitHub Enterprise.

so, they admit it's a specific dialect, not some kind of reference, common, or minimal set that all markdown renders are expected to implement.

How does that align with the readme support already there in GitHub?

GitHub's markdown dialect also supports things like:

If the suggestion is that NuGet should support GFM, then these are the things that have to work as well, as well as an expectation that every time GitHub add a new markdown feature, both nuget.org and NuGet client are expected to implement it as well. I don't think these features are valuable enough to warrant the complexity to have to re-implement them in both nuget.org and VS.

So, GitHub supporting something in their dialect isn't justification to me that NuGet should support it

What concerns me is that customers will expect the readme to render the same in VS as on nuget.org. If NuGet package readmes don't render "correctly" on github.com, that would be unfortunate, but it's less important to me because it's not a primary location for viewing package readmes, and not all packages use github.

If the desire is to use a single readme as both the package readme as well as the repo readme, that's understandable, so perhaps NuGet's readme dialect could be defined as a subset of GFM. Or perhaps we can encourage package authors to make the repo readme focused on contribution instructions, how to build and debug the repo locally, stuff that's not relevant to the package readme. It's still valuable for the repo readme to have a short introduction to what the package is and how to use it, but it can then have a link to nuget.org to see the full package readme, which can have more detailed information.

The alert format and markup is fairly standard at this point, I'd say...

That's fine. Then the NuGet server team can talk to the client team, get agreement that it's feasible to support the same formatting in both. I'm just saying the server team shouldn't extend the readme markdown dialect without talking to the client team.

I'm not saying don't do it. But I believe that customers are going to get a bad experience if a package author who doesn't use VS, or doesn't test their readme in VS, writes a readme that doesn't render correctly in VS. I'm sure that customers who notice the difference will report bugs to us. What should our response be in such a case? I don't want extra support costs just because a small number of package authors of open source packages on github, want to use the readme to render the same on github and nuget.org.

Therefore, I believe the readme should be considered part of the "protocol" that should have both client and server teams agree to, not one team adding it without even talking to the other team.

Copy link
Author

Choose a reason for hiding this comment

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

If the suggestion is that NuGet should support GFM, then these are the things that have to work as well, as well as an expectation that every time GitHub add a new markdown feature, both nuget.org and NuGet client are expected to implement it as well.

Not at all. But alerts are a very common documentation aid and it would be entirely unproductive to make up a brand new format, or worse, continue to NOT support it at all when it's ubiquitous even in MS learn site.

So, GitHub supporting something in their dialect isn't justification to me that NuGet should support it

The main reason would be familiarity for OSS authors, IMHO.

What concerns me is that customers will expect the readme to render the same in VS as on nuget.org.

What concerns me is that you may come up with an entirely incompatible way just for the purposes of your "nuget markdown" and force authors to learn yet another flavor rather than a subset of what they already know.

If the desire is to use a single readme as both the package readme as well as the repo readme, that's understandable, so perhaps NuGet's readme dialect could be defined as a subset of GFM.

Makes sense to be. The supported URLs in links are already one such case.

The alert format and markup is fairly standard at this point, I'd say...

That's fine.

That was the point of this PR. To showcase that it's trivial to add support for it on the gallery. You could just render plain blockquotes in VS without styling in v1 for readmes of that simplifies things. I doubt anyone will have high expectations for that readme visualization since it's a non existent feature at the moment.

Copy link

Choose a reason for hiding this comment

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

The MarkdownPreview component we're using doesn't currently allow for the alerts. I can check with the team that owns the component to see the feasibility of adding the functionality.

.UseReferralLinks("noopener noreferrer nofollow")
.UseAutoIdentifiers()
.UseEmphasisExtras(EmphasisExtraOptions.Strikethrough)
Expand Down Expand Up @@ -292,4 +293,4 @@ private RenderedMarkdownResult GetHtmlFromMarkdownMarkdig(string markdownString,
}
}
}
}
}
15 changes: 13 additions & 2 deletions tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -105,12 +105,23 @@ public void EncodesHtmlInMarkdownWithAdaptiveHeader(string originalMd, string ex
[InlineData("![](http://www.otherurl.net/fake.jpg)", "<p><img src=\"https://www.otherurl.net/fake.jpg\" alt=\"\" /></p>", true, false)]
[InlineData("![](http://www.otherurl.net/fake.jpg)", "<p><img src=\"https://www.otherurl.net/fake.jpg\" class=\"img-fluid\" alt=\"alternate text is missing from this package README image\" /></p>", true, true)]
[InlineData("## License\n\tLicensed under the Apache License, Version 2.0 (the \"License\");", "<h3 id=\"license\">License</h3>\n<pre><code>Licensed under the Apache License, Version 2.0 (the &quot;License&quot;);\n</code></pre>", false, true)]
[InlineData(
kzu marked this conversation as resolved.
Show resolved Hide resolved
"""
> [!NOTE]
> This is a note
""",
"""
<div class="markdown-alert markdown-alert-note alert alert-primary" role="alert">
<p class="mb-0">This is a note</p>
</div>
""",
false, true)]
lyndaidaii marked this conversation as resolved.
Show resolved Hide resolved
public void ConvertsMarkdownToHtml(string originalMd, string expectedHtml, bool imageRewriteExpected, bool isMarkdigMdRenderingEnabled)
{
_featureFlagService.Setup(x => x.IsMarkdigMdRenderingEnabled()).Returns(isMarkdigMdRenderingEnabled);
_featureFlagService.Setup(x => x.IsImageAllowlistEnabled()).Returns(false);
var readMeResult = _markdownService.GetHtmlFromMarkdown(originalMd);
Assert.Equal(expectedHtml, readMeResult.Content);
Assert.Equal(expectedHtml.Replace("\r\n", "\n"), readMeResult.Content.Replace("\r\n", "\n"));
Assert.Equal(imageRewriteExpected, readMeResult.ImagesRewritten);
}

Expand Down