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

feat(markdown): add support for GFM noteblocks proposal #10168

Merged
merged 10 commits into from
May 2, 2024

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Dec 11, 2023

Summary

This PR adds support for the GFM noteblocks proposal by performing the following changes:

  • Adds parsing support for the GFM noteblocks proposed syntax (see link above), performing transformations to add the magic text, leaving no noticeable differences
    • The callout type has also been added, even though the GFM proposal does not include such a type
    • The text for the three new noteblock types was localized using machine translation for all other locales
  • Adds code for the tip, important and caution types when we're ready to add them

Screenshots

Before

image

After

image image

How did you test this change?

Markdown used:

## MDN Style

> **Note:** Note!

> **Tip:** A helpful tip

> **Important:** Something important

> **Warning:** Be weary of this!

> **Caution:** Don't set this thing!

> **Callout:** Some call to action!

---

## GFM Proposal Style

> [!NOTE]
> Note!

> [!TIP]
> A helpful tip

> [!IMPORTANT]
> Something important

> [!WARNING]
> Be weary of this!

> [!CAUTION]
> Don't set this thing!

> [!CALLOUT]
> Some call to action!

@queengooborg queengooborg requested a review from a team as a code owner December 11, 2023 15:11
@github-actions github-actions bot added the markdown markdown related issues and pull requests label Dec 11, 2023
@caugner caugner added the involves: UX Requires the attention of the UX team. label Dec 11, 2023
@wbamberg
Copy link
Collaborator

I'm in favour of using the new GFM syntax, but I don't know why people want so many different kinds of admonitions. Warning, caution, note, tip, important... people will have no idea when to use which. I think it would be better to adopt this syntax for the admonitions we currently support and disallow the rest. More is not always better.

@teoli2003
Copy link
Contributor

More is not always better.

Yes, and if more is needed, we can always open a PR and have a discussion about the pertinence of it.

I also like using more standard syntax.

@queengooborg
Copy link
Collaborator Author

I've just pushed a commit to stick with the three existing noteblock types, but I've kept the styling, new icons, etc. in place so that we can easily add them in if/when we want to!

@queengooborg queengooborg changed the title Add support for GFM noteblocks proposal feat: Add support for GFM noteblocks proposal Dec 15, 2023
@github-actions github-actions bot added the idle label Jan 24, 2024
@fiji-flo
Copy link
Contributor

fiji-flo commented Apr 4, 2024

He we're (@caugner @argl) are fine with migrating to the new syntax. However, if we only support *Note", "Warning" and "Callout" let's remove support for the others. And I'd need some comment from the content team (@Rumyra). Would it then be okay to bulk change mdn/content and mdn/translated-content?

Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

/cc @mdn/yari-content-zh

I'm +1 for this, as this would solve the problem where we currently have to add spaces between the prefix and the content to make sure that the markdown parser can correctly handle the bold syntax:

The follow markdown content could not be parsed as we expected:

> **备注:**你好世界

Result:

<p>**备注:**你好世界</p>

markdown/m2h/handlers/index.ts Outdated Show resolved Hide resolved
@wbamberg
Copy link
Collaborator

wbamberg commented Apr 4, 2024

We'd also need a corresponding update to https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Howto/Markdown_in_MDN.

@queengooborg
Copy link
Collaborator Author

queengooborg commented Apr 4, 2024

let's remove support for the others.

The other three are disabled, so only the three we actively use are supported. There is simply code and assets available for when and if we want to enable them!

Would it then be okay to bulk change mdn/content and mdn/translated-content?

Yes, we could most definitely do a bulk update! Since this PR doesn't remove support for the "MDN syntax", we can perform the update gradually too. We can then follow up and drop support for the old notes syntax if we want to!

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

We (@argl @fiji-flo @caugner) took a look today and decided to approve the changes to support the GFM noteblocks proposal syntax for the three existing notecard types (note, warning, callout), but to reject the other changes preparing for hypothetically adding support for the other types.

Can you please revert the changes regarding the other noteblock types? We will then give it another review and merge the PR asap.

@caugner caugner marked this pull request as draft April 4, 2024 20:32
@caugner caugner changed the title feat: Add support for GFM noteblocks proposal feat(markdown): add support for GFM noteblocks proposal Apr 4, 2024
@queengooborg queengooborg marked this pull request as ready for review April 4, 2024 20:44
@queengooborg
Copy link
Collaborator Author

The code from other noteblocks has now been removed -- I'll create another PR that includes that spliced-out code/data as a sort of proposal, so if we do want to add them in, we can merge that PR without needing to redo any existing work!

(By the way, no need to set my PRs as drafts when requesting reviews -- I get to them pretty quickly. :P)

@github-actions github-actions bot removed the idle label Apr 5, 2024
Comment on lines +30 to +31
listMsgObj[msgName]["msgid"],
listMsgObj[msgName]["msgstr"][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allows for a much easier lookup using GFM notecard keywords. The logic is reversed when using MDN-flavored syntax, so it iterates through each entry to find the applicable type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to add (key, value) and (value, key) to the map to allow both directions, or is there a risk for a collision? In either case, let's add a comment above to give an example what msgid and msgstr[0] refer to to help.

Also, let's update the JSDoc of the method to clearly indicate what the returning map contains.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might make sense to have variables for both formats, but I feel that if we are planning to migrate to GFM syntax, then I think it makes more sense to just have the one format, as it will make removal of the old syntax parsing much quicker.

markdown/m2h/handlers/index.ts Show resolved Hide resolved
}

function getNotecardType(node: any, locale: string): NotecardType | null {
const types = ["note", "warning", "callout"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we shouldn't support callout here, if it isn't part of the GFM noteblocks proposal (and therefore not rendered by GitHub):

[!CALLOUT]
Callout

Important

Crucial information necessary for users to succeed.

Warning

Critical content demanding immediate user attention due to potential risks.

@mdn/core-yari-content What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: if we remove support for callouts in the GFM syntax, we'd have to special-case it for the original syntax. If the decision is not to support callouts in GFM syntax, then we should overhaul all existing callouts too.

@caugner caugner self-assigned this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
involves: UX Requires the attention of the UX team. markdown markdown related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants