-
Notifications
You must be signed in to change notification settings - Fork 508
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
Conversation
The code looks fine. I wanted to have a second iteration about going ahead with using this syntax. We have two 3 options:
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. |
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. |
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
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. |
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:
...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. |
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. |
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. |
@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? |
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. |
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
As a member of the |
There was a problem hiding this 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 💪
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:
Before
After