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.

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

Readme: Add support for NOTE, TIP, WARNING, IMPORTANT, and CAUTION notations #10125

Open
kzu opened this issue Aug 13, 2024 · 11 comments
Open

Readme: Add support for NOTE, TIP, WARNING, IMPORTANT, and CAUTION notations #10125

kzu opened this issue Aug 13, 2024 · 11 comments

Comments

@kzu
Copy link

kzu commented Aug 13, 2024

Related Problem

No response

The Elevator Pitch

Highlighting important parts of a package documentation is crucial to creating a more engaging experience.

In GitHub, the following readme markup:

> [!NOTE]
> This is a note

> [!TIP]
> This is a tip

> [!WARNING]
> This is a warning

> [!IMPORTANT]
> This is important

> [!CAUTION]
> This is a caution

Renders as follows
image

This is how the gallery renders that today: https://www.nuget.org/packages/Observable/0.2.0#readme-body-tab

image

Source for the examples: https://github.com/kzu/Observable

Additional Context and Details

Bringing nuget.org more aligned with GitHub markdown support is a good thing for OSS authors since it allows us to keep a unified readme in both places, lowering maintenance costs and avoiding duplication just to satisfy nuget.org restrictions on formatting.

@kzu kzu added the feature-request Customer feature request label Aug 13, 2024
@kzu kzu changed the title [Feature]: Add support for NOTE, TIP, WARNING, IMPORTANT, CAUTION and RECOMM [Feature]: Add support for NOTE, TIP, WARNING, IMPORTANT, and CAUTION Aug 13, 2024
@JonDouglas
Copy link
Contributor

JonDouglas commented Aug 13, 2024

Looks like this is possible w/ markdig today:

https://github.com/xoofx/markdig/blob/master/src/Markdig.Tests/Specs/AlertBlockSpecs.md

@erdembayar erdembayar changed the title [Feature]: Add support for NOTE, TIP, WARNING, IMPORTANT, and CAUTION Readme: Add support for NOTE, TIP, WARNING, IMPORTANT, and CAUTION notations Aug 20, 2024
@erdembayar erdembayar added this to the Backlog milestone Aug 20, 2024
@erdembayar
Copy link
Contributor

See https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1605 for Note and Tip ones.

image

@kzu
Copy link
Author

kzu commented Sep 4, 2024

I could give it a shot at sending a PR if it would help this ship sooner than later 🙏 . Just let me know and I can figure it out.

Thanks!

@erdembayar
Copy link
Contributor

Sure, you're welcome to create a PR. Please start with just one instead of all. If the first one is accepted, then we can proceed with the others.

Happy coding! 🧑‍💻

@erdembayar
Copy link
Contributor

erdembayar commented Sep 4, 2024

Please note that sometimes we ask for a spec review before starting implementation if it is going to change current behavior (e.g. how customers perceive), or if it will introduce any breaking changes or security concerns (for example, rendering requiring a new package, URL, or network call). Something like this: NuGet/Home#13673

@kzu
Copy link
Author

kzu commented Sep 5, 2024

Oh boy is this going to be "interesting":

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
  <PropertyGroup>
    <UseNuGetBuildExtensions>true</UseNuGetBuildExtensions>
    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>

I thought I was never going to see that legacy format ever again :(

@erdembayar
Copy link
Contributor

Oh boy is this going to be "interesting":

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
  <PropertyGroup>
    <UseNuGetBuildExtensions>true</UseNuGetBuildExtensions>
    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>

I thought I was never going to see that legacy format ever again :(

It shouldn't be hard to migrate, there're tools for doing that, recently some community members migrated some for us (#10000)

@kzu
Copy link
Author

kzu commented Sep 5, 2024

And, dev branch build fails due to #10166. 😿

Added this to the relevant .csproj.user for now:

    <TreatWarningsAsErrors>false</TreatWarningsAsErrors>

@kzu
Copy link
Author

kzu commented Sep 5, 2024

Quick Q @erdembayar: does the nuget.org use markdig rendering? (starting from ConvertsMarkdownToHtml)

kzu added a commit to kzu/NuGetGallery that referenced this issue Sep 5, 2024
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.

Partial fix for NuGet#10125
@kzu
Copy link
Author

kzu commented Sep 5, 2024

Ok, with an update to Markdig and a one-liner .UseAlertBlocks(), the following inline data passes for the mentioned test:

[InlineData(
    """
    > [!NOTE]
    > This is a note
    """,
    """
    <div class="markdown-alert markdown-alert-note alert alert-primary" dir="auto">
    <p class="markdown-alert-title" dir="auto"><svg class="octicon octicon-info mr-2" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="M0 8a8 8 0 1 1 16 0A8 8 0 0 1 0 8Zm8-6.5a6.5 6.5 0 1 0 0 13 6.5 6.5 0 0 0 0-13ZM6.5 7.75A.75.75 0 0 1 7.25 7h1a.75.75 0 0 1 .75.75v2.75h.25a.75.75 0 0 1 0 1.5h-2a.75.75 0 0 1 0-1.5h.25v-2h-.25a.75.75 0 0 1-.75-.75ZM8 6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"></path></svg>Note</p>
    <p dir="auto">This is a note</p>
    </div>
    """,
    false, true)]

That matches (almost exactly, except for alert alert-primary in the outer div) the html in github.com.

All that remains is where to put the styles.

kzu added a commit to devlooped/ThisAssembly that referenced this issue Sep 10, 2024
Since it's still not supported in nuget.org and we now aggregate this readme into the meta-package nuget.

Pending on NuGet/NuGetGallery#10125
kzu added a commit to kzu/NuGetGallery that referenced this issue Sep 13, 2024
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
kzu added a commit to kzu/NuGetGallery that referenced this issue Oct 15, 2024
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
@zivkan
Copy link
Member

zivkan commented Oct 22, 2024

IMO this should be validated with the NuGet Client team to ensure that it'll be possible to support in Visual Studio. Markdown features that only work on nuget.org and not Visual Studio won't be good for the ecosystem.

cc @jgonz120

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

No branches or pull requests

4 participants