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(mdx-loader): support nested admonitions #8303

Merged
merged 7 commits into from Nov 23, 2022
Merged

Conversation

lebalz
Copy link
Contributor

@lebalz lebalz commented Nov 8, 2022

Pre-flight checklist

Motivation

See #8302 - support nested admonitions

Test Plan

Test links

Related issues/PRs
#8302

@lebalz lebalz requested a review from slorber as a code owner November 8, 2022 14:53
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 8, 2022
@netlify
Copy link

netlify bot commented Nov 8, 2022

[V2]

Name Link
🔨 Latest commit 3c9e769
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/637e6e8280f4fe0008cb7c2b
😎 Deploy Preview https://deploy-preview-8303--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 59 🟢 97 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 74 🟢 100 🟢 100 🟢 100 🟢 90 Report

@lebalz lebalz changed the title introduce nested admonitions fix(admonitions): support nested admonitions Nov 8, 2022
@slorber
Copy link
Collaborator

slorber commented Nov 9, 2022

Hey

Thanks for the PR

As part of the migration to MDX 2.0 (WIP - #8288) we will likely replace all this code with remark-directive so I'm not sure we are going to merge this PR for now. I'll keep it open but the goal is to get rid of the code. I guess it should be possible to support nested admonitions with the remark plugin.

@Josh-Cena
Copy link
Collaborator

I think it's still a valuable fix to get into 2.x? I won't anticipate 3.0 to ship in a few weeks, not to mention its wide adoption.

@slorber
Copy link
Collaborator

slorber commented Nov 10, 2022

I think it's still a valuable fix to get into 2.x? I won't anticipate 3.0 to ship in a few weeks, not to mention its wide adoption.

We'll see.

I'd like to see how to implement this in 3.x / MDX 2 before adding such new feature. In case it's complex or impossible to implement in 3.x, I'd rather not introduce a new feature to remove it later, nor to block / delay 3.x because of this new feature.

Let's keep this open and see how we can have it both in 2.x and 3.x

@slorber
Copy link
Collaborator

slorber commented Nov 18, 2022

As part of #8288 we want to use the more flexible and generic remark-directive system to provide such features.

The nesting is possible, but works similarly to code blocks by adding more : for parent elements.

Here's a preview:
https://deploy-preview-8288--docusaurus-2.netlify.app/docs/markdown-features/admonitions/#specifying-title

:::::note{title="Your title"}

Some **content** with _Markdown_ `syntax`.

::::note[Your nested Title]

:::note Your very nested Title (does not work yet :s)

Some **content** with _Markdown_ `syntax`.

:::

Some **content** with _Markdown_ `syntax`.

::::

hey

:::::

CleanShot 2022-11-18 at 18 09 29@2x

If we add this feature in v2 I'd rather have the exact same syntax instead of always using :::

That way users do not have to change anything when upgrading and there's no extra breaking change to explain.

Can you update the code to support 4/5+ colons please?

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Nov 18, 2022
@lebalz
Copy link
Contributor Author

lebalz commented Nov 20, 2022

Thanks for the review and the suggestions. I changed the algorithm which keeps track of the nesting level. Now it handles nested admonitions the way remark-directive does, e.g. with 4/5/+ colons...

@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Nov 23, 2022
@slorber
Copy link
Collaborator

slorber commented Nov 23, 2022

LGTM thanks 👍

@slorber slorber changed the title fix(admonitions): support nested admonitions fix(mdx-loader): support nested admonitions Nov 23, 2022
@slorber slorber merged commit b016686 into facebook:main Nov 23, 2022
slorber pushed a commit that referenced this pull request Jan 26, 2023
@slorber slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support nested admonitions
4 participants