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(theme-classic): standalone Admonition component #5848

Merged
merged 3 commits into from
Dec 20, 2021
Merged

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Nov 1, 2021

Motivation

We should prepare to ditch remark-admonitions because:

  1. It is very unmaintained;
  2. It is coupled to Infima;
  3. It isn't compatible with Remark v13.

In addition, we should have a standalone admonition element for people to use outside of Markdown.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

https://deploy-preview-5848--docusaurus-2.netlify.app/docs/next/markdown-features/admonitions/#usage-in-jsx

Blocking issues

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 1, 2021
@netlify
Copy link

netlify bot commented Nov 1, 2021

✔️ [V2]

🔨 Explore the source changes: fc79d30

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61bee0f69de26a0007ec62f1

😎 Browse the preview: https://deploy-preview-5848--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Nov 1, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 95
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5848--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Nov 1, 2021

Size Change: 0 B

Total Size: 652 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 37.8 kB
website/build/assets/css/styles.********.css 101 kB
website/build/assets/js/main.********.js 484 kB
website/build/index.html 29.6 kB

compressed-size-action

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Nov 1, 2021
@lex111
Copy link
Contributor

lex111 commented Nov 3, 2021

We can fork remark-admonitions and improve it by making it compatible with Remark 13, etc. It's actually a handy shortcut for inserting admonitions, many users already use it, and are unlikely to want to opt-out from it.
So having React implementation of this element seems superfluous code duplication, especially since it was intended solely for use in content MD files.

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Nov 3, 2021

We can fork remark-admonitions and improve it by making it compatible with Remark 13, etc. It's actually a handy shortcut for inserting admonitions, many users already use it, and are unlikely to want to opt-out from it.

That's what I plan to do afterwards—fork remark-admonitions, either in our repo or in the docusaurus org if we ever see its birth. Maybe I didn't make the intention clear, but I never want to make users use JSX for admonitions like Tabs do, I just plan to swap out the underlying mechanism. I believe our impl of remark-admonitions should make it transpile to a @theme/Admonition comp rather than hard-coded markup with interleaved Infima references, because that's not scalable to multiple themes, hence this PR.

So having React implementation of this element seems superfluous code duplication, especially since it was intended solely for use in content MD files.

I was actually asked not a single time "how to use admonitions outside Markdown" and I also wondered about that from time to time. It's not a bad move IMO given my reasonings in the PR description about why the current state of remark-admonitions is not ideal.

@slorber
Copy link
Collaborator

slorber commented Nov 3, 2021

We should probably migrate to https://github.com/remarkjs/remark-directive because it may be standardized in CommonMark someday (I'm not sure how this std process works but there are extensive discussions here: https://talk.commonmark.org/t/generic-directives-plugins-syntax/444)

This directive plugin is supported Titus so I'd prefer to use it for admonitions (and likely other use-cases too) rather than migrating the existing admonitions to Remark 13.

I'd also prefer to not introduce this extra theme component until we do this migration, because that migration + the better support for CommonMark is likely to help us define a better API surface for this component.

So, overall I'm 👍 for having a @theme/Admonitions component, but I don't think it's good timing and we should migrate to MDX 2.0 first.

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Nov 3, 2021

We should probably migrate to https://github.com/remarkjs/remark-directive because it may be standardized in CommonMark someday (I'm not sure how this std process works but there are extensive discussions here: https://talk.commonmark.org/t/generic-directives-plugins-syntax/444)

I'm aware of that plugin and also the fact that it's MDX 2.0. To migrate to MDX v2 we need ESM hence #5816. However remark-directives from my understanding is still a higher-level abstraction and in order to make something solid we need to build our impl on top of that. I may be wrong—never checked how it actually works

I'd also prefer to not introduce this extra theme component until we do this migration, because that migration + the better support for CommonMark is likely to help us define a better API surface for this component.

I'm not aware of how CM helps us with internal implementations🌚 Suppose we have another tailwind theme. How would we go about reconciling them without introducing a theme comp?

Also, I'm just wishing for an OOTB admonition comp that I can use for my React page. It's weird to use many documentation features in React but why not

Edit. After re-reading your comment I think you mean we should have different props for the comp?

@slorber
Copy link
Collaborator

slorber commented Nov 3, 2021

I'm in favor of having this new @theme/Admonition comp for the same reasons as you do.

I just want to delay introducing that comp for now, but rather do this later to ensure that the comp's API is compatible with our MDX 2 setup

@Josh-Cena
Copy link
Collaborator Author

Ah, no problem. We have a lot of PRs depending on each other now :D Maybe we should make a roadmap or something to keep track of them. A simple milestone doesn't seem enough

@Josh-Cena Josh-Cena added the status: blocked This issue is blocked by another issue or external dep and can't be pushed further. label Nov 18, 2021
@Josh-Cena Josh-Cena removed the status: blocked This issue is blocked by another issue or external dep and can't be pushed further. label Dec 19, 2021
@Josh-Cena
Copy link
Collaborator Author

So interestingly, as MDX v2 migration is postponed to v3, this PR can now be pushed forward because the API would at least be stable for v2

@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 20, 2021
@slorber
Copy link
Collaborator

slorber commented Dec 20, 2021

After further study I don't think adding this component now will be more challenging anyway so it seems safe enough to add now

thanks

@slorber slorber merged commit cb42652 into main Dec 20, 2021
@slorber slorber deleted the jc/admonition-comp branch December 20, 2021 16:51
@Josh-Cena Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants