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

Convert noteblocks for glossary folder #35067

Merged
merged 5 commits into from
Jul 25, 2024
Merged

Convert noteblocks for glossary folder #35067

merged 5 commits into from
Jul 25, 2024

Conversation

queengooborg
Copy link
Collaborator

This PR converts the noteblocks for the 'glossary' folder to GFM syntax, using a conversion script.

This PR converts the noteblocks for the 'glossary' folder to GFM syntax, using a [conversion script](https://github.com/queengooborg/mdn-toolkit/blob/main/upgrade-noteblock.js).
@queengooborg queengooborg requested a review from a team as a code owner July 25, 2024 16:54
@queengooborg queengooborg requested review from hamishwillee and removed request for a team July 25, 2024 16:54
@github-actions github-actions bot added Content:Glossary Glossary entries size/m [PR only] 51-500 LoC changed labels Jul 25, 2024
Copy link
Contributor

github-actions bot commented Jul 25, 2024

Preview URLs (24 pages)
External URLs (8)

URL: /en-US/docs/Glossary/Forbidden_header_name
Title: Forbidden header name


URL: /en-US/docs/Glossary/First_meaningful_paint
Title: First Meaningful Paint


URL: /en-US/docs/Glossary/Serializable_object
Title: Serializable object


URL: /en-US/docs/Glossary/HTML5
Title: HTML5


URL: /en-US/docs/Glossary/TLS
Title: Transport Layer Security (TLS)


URL: /en-US/docs/Glossary/Breadcrumb
Title: Breadcrumb

(comment last updated: 2024-07-25 21:58:51)

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

@queengooborg There is a rendering difference with an extra blank line:

image

And I don't find it acceptable to make this change, because it doesn't appear revertible to me (once we make this workaround it would be hard to discover all places that are intended as workaround, vs. those intended to start a new paragraph). We may have to wait on the Prettier bug.

In your conversion script, could you not do conversion for any note that starts with non-letters?

In the meantime, I think we should revert all notes with the > \[![A-Z]+\]\n>\n form to the previous state (except callouts, which were supposed to look like that). There aren't many on main.

@queengooborg
Copy link
Collaborator Author

Agh, okay, there is a difference in rendering, whoops -- I genuinely thought that I had added logic to make the rendering identical with a newline in between... 🤦‍♀️

In your conversion script, could you not do conversion for any note that starts with non-letters?

I could make that change, but, uh...I already ran the script on all the files locally, fixed issues related to Prettier/Markdownlint malformatting, and submitted PRs for the vast majority of the docs... 😅


Truth be told, I don't think that this rendering difference is too much of an issue if it was styled better (AKA, more like how GitHub renders the noteblocks). But that's just my opinion and would be something that should be discussed before proceeding with!

@Josh-Cena
Copy link
Member

Josh-Cena commented Jul 25, 2024

@queengooborg Fear not, I'm checking out each of your branches and running a regex search on each one, so you can just look at the cases I pointed out. If you want to do a self-check first, you can search > \[!(NOTE|WARNING)+\]\n>\n|(NOTE|WARNING)\] >.

Truth be told, I don't think that this rendering difference is too much of an issue if it was styled better (AKA, more like how GitHub renders the noteblocks). But that's just my opinion and would be something that should be discussed before proceeding with!

Agreed; I would be equally happy if we render like GitHub (put "Note" on a separate line).

@Josh-Cena Josh-Cena merged commit 9409e72 into main Jul 25, 2024
9 checks passed
@Josh-Cena Josh-Cena deleted the glossary branch July 25, 2024 21:58
@hochan222
Copy link
Member

hochan222 commented Jul 27, 2024

@queengooborg Hello.

I recently confirmed this change in the locale-ko. Is there a locale guide? 🙇

@queengooborg
Copy link
Collaborator Author

queengooborg commented Jul 27, 2024

There isn't really a guide for performing the conversion, even for English content -- but there is a script I wrote, which can be found in the description of this PR!

Edit: thinking about it again, I'd say that the closest thing to a guide would be the Markdown in MDN page -- so completing mdn/translated-content#14373 would help!

@hochan222
Copy link
Member

I asked because I wanted to change NOTE to Korean, but it looks like I should use NOTE as is!

I understand. I am leaving some of the contents of the MDN Markdown guide for other people. Thank you :)

image
> [!NOTE]  
> Highlights information that users should take into account, even when skimming.

> [!TIP]
> Optional information to help a user be more successful.

> [!IMPORTANT]  
> Crucial information necessary for users to succeed.

> [!WARNING]  
> Critical content demanding immediate user attention due to potential risks.

> [!CAUTION]
> Negative potential consequences of an action.

@queengooborg
Copy link
Collaborator Author

That's correct -- the magic keywords should stay in English now, rather than being localized. This ensures consistency across locales and better compatibility with Markdown renderers besides Yari (like GitHub's web editor)!

@hochan222
Copy link
Member

Agreed; I would be equally happy if we render like GitHub (put "Note" on a separate line).

@queengooborg, @Josh-Cena How about adding prettier ignore?

<!-- prettier-ignore-start -->
> [!NOTE]
> [Here is another example without Canvas or external libraries.](https://jsfiddle.net/jlr7245/teb4znk0/20/)
<!-- prettier-ignore-end -->

@Josh-Cena
Copy link
Member

I don't find it necessary and it adds more noise than just keeping it unmigrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Glossary Glossary entries size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants