Add support for markdown > alerts via markdig #10173

kzu · 2024-09-05T20:33:40Z

Partial fix for #10125

Markdig supports alert blocks as documented at https://github.com/xoofx/markdig/blob/master/src/Markdig.Tests/Specs/AlertBlockSpecs.md.

Bumping markdig and adding .UseAlertBlocks() now renders the expected html.

Not sure how to go about adding the styles as needed.

tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs

kzu · 2024-09-05T20:54:25Z

I guess as a test PR, it showcases that for someone that can actually run the site, it should be fairly straightforward to add support for this :)

lyndaidaii · 2024-09-05T21:16:20Z

I guess as a test PR, it showcases that for someone that can actually run the site, it should be fairly straightforward to add support for this :)

Thank you for your contribution. I could help to test it.

src/NuGetGallery/Content/Site.css

erdembayar · 2024-09-05T21:49:08Z

I guess as a test PR, it showcases that for someone that can actually run the site, it should be fairly straightforward to add support for this :)

Thank you for your contribution. I could help to test it.

Thanks, Lynn. Please add screenshot here.

erdembayar · 2024-09-10T20:37:19Z

@lyndaidaii
Have you had chance to test this? If not then I can do it, please let me know.

erdembayar · 2024-09-10T20:42:44Z

src/NuGetGallery/Content/Site.css

@@ -1374,20 +1374,20 @@ ul.accordion {
        }

        ul.accordion li.accordion-item .accordion-expand-link {
-            background: none!important;
+            background: none !important;


Also this change should have been done in .less file rather than this file?

I have no idea where things are in your code base. Feel free to close this issue which was mostly a demonstration that fixing the issue is mostly adding one line of code and whichever styles you want for the site 🙏

would you mind pointing at which .less file you're referring to?

@NuGet/gallery-team

Do you know which less file we need to make changes to? I never change any of them.

kzu · 2024-09-13T02:54:57Z

Test should be passing now. Screenshots below.

Dark mode:

Light mode;

kzu · 2024-09-16T15:04:03Z

any updates @erdembayar @lyndaidaii ?

erdembayar · 2024-09-16T15:07:53Z

any updates @erdembayar @lyndaidaii ?

Sorry, we'll review this week or at the latest next week. We have other priorities at the moment and need some time.

lyndaidaii · 2024-09-16T16:54:16Z

@lyndaidaii Have you had chance to test this? If not then I can do it, please let me know.

I had trouble in building gallery in local since we merged a couple of repos into one. My .NET version on dev is out of date now. Haven't checked in since then. If you could build gallery in local fine, could you help take a look at UI?

kzu · 2024-09-30T13:51:43Z

Any updates?

erdembayar · 2024-10-04T00:01:18Z

Any updates?

As you can see, unit tests are passing, which is good news. ~~However, I couldn't test the actual rendering view part because our Gallery app has a binding redirect issue due to a recent package update. We need to fix it first.~~ In the meantime, could you please upload the NuGet package with the readme you demoed to nuget.org or share the Readme.md file with me here? It would make my testing easier.

erdembayar · 2024-10-04T01:07:53Z

I tested your code after we fixed our code. Could you please rebase your branch using our dev latest branch?
With my testing it seems rendering is not really working. Could you please test it yourself?

I used below Readme.md


## About This Project

Hello this is readme file.

[!NOTE]
This is a note 2024

lyndaidaii · 2024-10-07T23:55:32Z

Looks like is missing some of css file.

kzu · 2024-10-08T00:30:35Z

@erdembayar the sample package I used is available as source (pretty bare, mind you) at https://github.com/kzu/Observable.

It's basically this readme: readme.md

And a nuspec like

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
  <metadata>
    <id>Observable</id>
    <version>0.2.0</version>
    <authors>Daniel Cazzulino</authors>
    <requireLicenseAcceptance>false</requireLicenseAcceptance>
    <license type="expression">MIT</license>
    <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
    <projectUrl>https://github.com/devlooped/Observable</projectUrl>
    <description>Sample.</description>
    <readme>readme.md</readme>
    <tags>dotnet csharp</tags>
  </metadata>
  <files>
    <file src="readme.md" target="readme.md" />
  </files>
</package>

Observable.0.2.0.nupkg.zip

which I ran nuget pack on.

kzu · 2024-10-08T00:38:38Z

I rebased, and I'm getting proper styles:

The app isn't requesting anything other than the fabric.css and bootstrap.min.css:

And those seem to be the default alert styles in bootstrap indeed:

https://getbootstrap.com/docs/3.4/components/#alerts

That's for the right version of bootstrap in use by the gallery:

erdembayar · 2024-10-10T23:26:58Z

I rebased, and I'm getting proper styles:

Did you push your rebased code? I don't see any commit from October. Git history shows only commits from beginning of Sep.

Thank you for your patience. I tested your change with https://github.com/kzu/Observable and it works as shown on your screenshot for all markdowns except for Note, which I had tested previously.

The Note should look like the one above. Currently, as shown on your screenshot, it has no coloring.

Additionally, could coloring needs to be stronger? To me, it looks a bit soft. It's not blocker, it's coming from bootstrap.

Markdig supports alert blocks as documented at https://github.com/xoofx/markdig/blob/master/src/Markdig.Tests/Specs/AlertBlockSpecs.md. Bumping markdig and adding `.UseAlertBlocks()` now renders the expected html. Fixes NuGet#10125

If we don't unset it, the default bootstrap p style turns dark mode illegible (uses white-ish color). This seems to be an issue specific to the very old version of Bootstrap being used (v3.x). When running the site locally, I could not find any other .css (or .less) files being built or linked from the page, so I placed the only needed style in the only other style file other than bootstrap.

kzu · 2024-10-15T17:24:15Z

@erdembayar what I meant is that the rebase didn't make any difference. Just in case, I just pushed anyways after a new rebase.

The NOTE will never look like that because there's no such built-in style for that in bootstrap. I have no idea where you put styles that aren't bootstrap, but if you tell me where, I can add one for NOTE since extending the alert styles is pretty easy in bootstrap. But I think this is going too far. I'm not a designer :).

The PR showcases how adding support for this is straightforward. The unit test shows that the proper classes are emitted. Now all that's left is for you to bring that note style that's used elsewhere in MS docs sites and put it somewhere you like. I didn't find any place with custom styles, which is why I just put something in the fabric.css, but I've no idea if that's the "right" place.

kzu · 2024-10-15T17:26:22Z

BTW, I'm happy to just copy over from github css the style for the above, but I've no idea if that's what you want design-wise for nuget.org :)

lyndaidaii · 2024-10-15T17:30:05Z

@kzu, thank you for your contribution. Great work. No worries at all. I will work on the style.

erdembayar · 2024-10-15T18:11:05Z

BTW, I'm happy to just copy over from github css the style for the above, but I've no idea if that's what you want design-wise for nuget.org :)

Thank you. It looks @lyndaidaii is going to take care of Note.

erdembayar · 2024-10-15T18:13:31Z

@kzu, thank you for your contribution. Great work. No worries at all. I will work on the style.

For your reference, I found actual Note sample used in our documentation which is also markdown format.
See bottom of https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1605, also as you can see coloring is Tip little bit different too.

kzu · 2024-10-15T18:14:49Z

Yeah, that looks better IMO

lyndaidaii · 2024-10-21T21:04:33Z

I updated CSS file for alert styling, here are how it looks like with these changes.

src/Bootstrap/dist/css/bootstrap-theme.css

tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs

erdembayar

@lyndaidaii
Thanks for driving this to a conclusion! 👏
Maybe we can to make coloring more stronger for Alert, Tip etc, not blocking, we can do it in another PR.
Please wait for a while to let other team members review.

zivkan · 2024-10-22T21:53:01Z

src/NuGetGallery/Services/MarkdownService.cs

@@ -208,6 +208,7 @@ private RenderedMarkdownResult GetHtmlFromMarkdownMarkdig(string markdownString,
                .UseTaskLists()
                .UseEmojiAndSmiley()
                .UseAutoLinks()
+                .UseAlertBlocks()


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:

[Feature] Render package README from local storage in the PM UI NuGet.Client#6110

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

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...

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:

collapsed sections

mermaid js diagrams

mathematical expressions

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.

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.

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.

erdembayar · 2024-12-03T18:31:11Z

We need to have cross team discussion between client and server team on how to roll out this change. @lyndaidaii @jgonz120

lyndaidaii · 2024-12-03T23:01:21Z

We need to have cross team discussion between client and server team on how to roll out this change. @lyndaidaii @jgonz120

I sent out email a while back. I will ping that email thread again.

erdembayar · 2025-01-14T18:21:39Z

We need to have cross team discussion between client and server team on how to roll out this change. @lyndaidaii @jgonz120

I sent out email a while back. I will ping that email thread again.

@lyndaidaii
Do you have any recent update?

kzu requested a review from a team as a code owner September 5, 2024 20:33

erdembayar reviewed Sep 5, 2024

View reviewed changes

tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs Outdated Show resolved Hide resolved

lyndaidaii self-assigned this Sep 5, 2024

erdembayar reviewed Sep 5, 2024

View reviewed changes

src/NuGetGallery/Content/Site.css Outdated Show resolved Hide resolved

erdembayar reviewed Sep 10, 2024

View reviewed changes

kzu force-pushed the dev branch 2 times, most recently from 101fa6a to 86eeeeb Compare September 13, 2024 03:11

erdembayar self-assigned this Oct 1, 2024

kzu added 2 commits October 15, 2024 14:16

kzu force-pushed the dev branch from 86eeeeb to 5cd5170 Compare October 15, 2024 17:17

erdembayar removed their assignment Oct 15, 2024

update css style

3bf38d8

lyndaidaii reviewed Oct 21, 2024

View reviewed changes

src/Bootstrap/dist/css/bootstrap-theme.css Show resolved Hide resolved

erdembayar reviewed Oct 21, 2024

View reviewed changes

tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs Outdated Show resolved Hide resolved

add unit test

0bdf47c

erdembayar approved these changes Oct 22, 2024

View reviewed changes

zivkan reviewed Oct 22, 2024

View reviewed changes

joelverhagen mentioned this pull request Nov 12, 2024

Pr 10173 #10204

Closed

Add support for markdown > alerts via markdig #10173

Are you sure you want to change the base?

Add support for markdown > alerts via markdig #10173

Conversation

kzu commented Sep 5, 2024 • edited by erdembayar Loading

kzu commented Sep 5, 2024

lyndaidaii commented Sep 5, 2024

erdembayar commented Sep 5, 2024

erdembayar commented Sep 10, 2024

erdembayar Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzu commented Sep 13, 2024 • edited Loading

kzu commented Sep 16, 2024

erdembayar commented Sep 16, 2024

lyndaidaii commented Sep 16, 2024 • edited Loading

kzu commented Sep 30, 2024

erdembayar commented Oct 4, 2024 • edited Loading

erdembayar commented Oct 4, 2024 • edited Loading

lyndaidaii commented Oct 7, 2024

kzu commented Oct 8, 2024

kzu commented Oct 8, 2024

erdembayar commented Oct 10, 2024 • edited Loading

kzu commented Oct 15, 2024

kzu commented Oct 15, 2024

lyndaidaii commented Oct 15, 2024

erdembayar commented Oct 15, 2024

erdembayar commented Oct 15, 2024

kzu commented Oct 15, 2024

lyndaidaii commented Oct 21, 2024 • edited Loading

erdembayar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Dec 3, 2024

lyndaidaii commented Dec 3, 2024

erdembayar commented Jan 14, 2025

kzu commented Sep 5, 2024 •

edited by erdembayar

Loading

erdembayar Sep 10, 2024 •

edited

Loading

kzu commented Sep 13, 2024 •

edited

Loading

lyndaidaii commented Sep 16, 2024 •

edited

Loading

erdembayar commented Oct 4, 2024 •

edited

Loading

erdembayar commented Oct 4, 2024 •

edited

Loading

erdembayar commented Oct 10, 2024 •

edited

Loading

lyndaidaii commented Oct 21, 2024 •

edited

Loading