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

fix(markdown): Fix GFM rendering if blank line right after magic keyword #11108

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

queengooborg
Copy link
Collaborator

Summary

This PR is a follow-up to #10168, which fixes the rendering of GFM noteblocks if there is a blank line after the magic keyword (which is required for callouts due to Prettier formatting).

Problem

See summary

Solution

The solution is simple: update the regex to remove the magic keyword from rendering to make the newline character afterwards optional, since a blank line in between the magic keyword and content generates multiple paragraphs instead of a newline character. Afterwards, any preceding blank lines are stripped for pretty rendering.


Screenshots

Test Markdown used:

> [!NOTE]
> Hello!

> [!WARNING]
> Hello!

> [!CALLOUT]
> Hello!

---

> [!NOTE]
>
> Hello!

> [!WARNING]
>
> Hello!

> [!CALLOUT]
>
> Hello!

Before

Screenshot 2024-05-11 at 17 28 32

After

Screenshot 2024-05-11 at 17 28 48

@queengooborg queengooborg requested a review from a team as a code owner May 12, 2024 00:32
@github-actions github-actions bot added the markdown markdown related issues and pull requests label May 12, 2024
@queengooborg
Copy link
Collaborator Author

@caugner @fiji-flo Would either of you mind reviewing and merging this PR soon? It fixes a critical bug that is preventing migration and mass usage of GFM syntax.

@fiji-flo
Copy link
Contributor

The code looks fine. I wanted to have a second iteration about going ahead with using this syntax. We have two 3 options:

  1. We use this proposal and create a situation where en-US renders fine with standard gfm but translated content is broken. This also means we depend on customizing existing gfm plugins in the future.
  2. We only support English terms and force translated content to use it (we can still display the localized term in the front-end). This is not in the spirit of markdown.
  3. We keep our custom implementation.

If everyone agrees on 1. we can go ahead, but I do wanna highlight that this really only helps for en-US and potentially makes porting this to the future harder.

@queengooborg
Copy link
Collaborator Author

Im a little confused about what's being asked here, but we have already agreed to go forward with the GFM syntax in a previous PR. This PR performs a critical bug fix.

Are you referring to the final rendering, or the magic keywords? If it's the magic keywords, the idea is that the keywords are left unchanged, as GFM only uses English terms, and it's unlikely that Markdown editors and viewers will support non-English terms as a result. If it's the final rendering, we render the proper localization for the document language when using GFM syntax.

@fiji-flo
Copy link
Contributor

I'm sorry for the confusion. Yes this is a bit of peddling back on the previous decision.

I was mistaken when looking at the initial PR and thought this would add also support for

> [!WARNUNG]
> this is warning in German

I think I wanted to take a step back and see what we want here. I'm not sure how much adoption this extension will get outside of gfm since the it seems controversial to add English keywords to markdown. It renders arguably worse without support compared to the old syntax we use.

I don't want to block this much longer (and really sorry that it took so long), I think I'm looking for some more positive signal from translators. @yin1999 pointed out that this will get rid of the white space issue, however this now needs a new line which would also solve the issue with the old version.

I'll tag in @wbamberg and @teoli2003 as they replied to the original PR and maybe we should ask some more translators. But if it's only me who has concerns I'm happy to land this and then let's regex all content. Again sorry for dragging this out for so long.

@wbamberg
Copy link
Collaborator

So am I right in thinking that the problem with the new GFM syntax is that in "our" note syntax, non-en-US people can use their language for the "Warning" keyword, as documented in https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Howto/Markdown_in_MDN#translated_warning:

> **Warnung:** So schreibt man eine Warnung.

...and this isn't supported in the GFM syntax? I do agree that this is a pity and that it's up to the translators to say how much of a regression this is. We did discuss it in the issue where we designed the MDN syntax: mdn/content#3483 (comment).

I have some other concerns about the GFM syntax. From the giant thread on it, in GitHub's implementation:

The second of these might be a bug? but the first seems like it might be by design? We can't tell because it isn't in the spec yet. Our implementation does not have these limitations AFAICS, so we are already nonconforming, but that means we have to keep a custom implementation of admonitions, which partly negates the value of using GitHub's syntax in the first place. So I suppose we could further diverge from their version, to allow non-en-US admonition keywords. But at some point it feels like, why adopt their syntax at all if we will diverge from it so much. It's tempting to suggest looking at directives instead, as proposed here: https://github.com/orgs/community/discussions/16925#discussioncomment-2791869.

@queengooborg
Copy link
Collaborator Author

Regarding the localizations, that is actually one of the reasons I wanted to migrate to GFM syntax in the first place and use only one magic keyword instead of many localized versions. I have found that various translators had translated the magic keyword differently (at least, in the past), so often times, noteblocks would be broken because a translator had picked "aviso" instead of "advertencia" (both are Spanish for "warning"). While it is nice to have the localizations of these keywords directly in the Markdown files, my personal feeling on it is that they're more problematic than helpful, at least from what I've seen.


I feel that this would be best to move into a discussion, however, rather than blocking a critical bugfix? We already have the infrastructure in place to use GFM syntax, and all this PR is doing is fixing an issue with the parsing.

@github-actions github-actions bot added the idle label Jul 13, 2024
@Josh-Cena
Copy link
Member

Can the team merge this as a simple bug fix, or tell us that there's no intention to use GFM syntax at all (which @fiji-flo's comment seems to suggest, pointing out "intrinsic problems" with this syntax)? There are PRs blocked on this in en-US and even if localization doesn't like it, en-US can start using this feature.

Note that I don't think you would ever get "built-in support" of this syntax in a GFM parser. GFM parsers parse things that are otherwise plain text, such as tables, footnotes, etc. This callout syntax is already well-formed Markdown and only needs to be endowed with extra semantics.

@fiji-flo
Copy link
Contributor

@Josh-Cena I'm tagging in @Rumyra

I'm fine merging this as long as we convert (happy to help) all to the new syntax, including translated content.

Do we all agree with that?

@Josh-Cena
Copy link
Member

I'm also +1 on merging this. Markdown is source code and I don't quite get why we need localizability of source code. We don't localize front matter keys either.

@yin1999
Copy link
Member

yin1999 commented Jul 19, 2024

I imagine this also helps with translated content, as new contributors may not familiar with these localized and fixed prefixes. It's easy to make a "note card" not highlighted due to the use of wrong symbols or the wrong words. The following errors are often encountered in zh-cn content:

  • Incorrect use of punctuation: incorrect use of : instead of
  • For note cards, the "wrong" word '注意' is used instead of '备注' (Similar to the problem mentioned by @queengooborg)

As a member of the l10n-zh team, I'm also +1 for the current changes :)

Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Thanks for the patience. Let's change all the content 💪

@fiji-flo fiji-flo merged commit 16d7b88 into mdn:main Jul 19, 2024
8 checks passed
@queengooborg queengooborg deleted the gfm branch July 19, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
markdown markdown related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants