fix(mdx-loader): properly unwrap mdxAdmonitionTitle placeholder #8246
Conversation
Josh-Cena
force-pushed
the
fix-admonition-title
branch
from
037606a
to
cd842a8
Compare
Josh-Cena
added
pr: bug fix
| @@ -51,7 +51,7 @@ exports[`admonitions remark plugin default behavior for custom keyword 1`] = ` | |||
|
|
|||
| exports[`admonitions remark plugin interpolation 1`] = ` | |||
| "<p>Test admonition with interpolated title/body</p> | |||
| <admonition type="tip"><mdxAdmonitionTitle>My <code>interpolated</code> <strong>title</strong> <button style={{color: "red"}} onClick={() => alert("click")}>test</mdxAdmonitionTitle><p><code>body</code> <strong>interpolated</strong> content</p></admonition>" | |||
| <admonition type="tip"><template data-admonition-title></template><p><code>body</code> <strong>interpolated</strong> content</p></admonition>" | |||
There was a problem hiding this comment.
This appears to be a Remark bug that the template content does not get serialized 🤔 The rendered output is fine, though. I'm not sure I should change the tag name just to make testing easier.
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Size Change: 0 B Total Size: 819 kB ℹ️ View Unchanged
|
|
Dogfood page: http://localhost:3000/docs/markdown-features/diagrams Another fix I find better (no remark bug at least) that also works is to use uppercase component name and add a placeholder component to MDXComponents: MDXAdmonitionTitle: (props) => <React.Fragment {...props} />,What I'd like to understand first is why we have the problem on this specific case, and not on all other lowercase MDX elements like admonition, mermaid, highlight... I think I saw this warning in the past in other cases but can't remember. I guess it's likely related to the order in which remark plugins are applied in mdx compiler itself. I'm not sure our test snapshots include the exact same processing pipeline compared to MDX so it's hard to debug. |
|
Other lowercase names have their corresponding MDX component mappings, so MDX is able to please React about these tags. OTOH |
| const rest = <>{items.filter((item) => item !== mdxAdmonitionTitle)}</>; | ||
| return { | ||
| mdxAdmonitionTitle, | ||
| mdxAdmonitionTitle: mdxAdmonitionTitle?.props.children, |
There was a problem hiding this comment.
Hey
Actually the template trick seems un-necessary: you just found the issue here
See prod markup:
https://docusaurus.io/tests/pages/markdownPageTests#admonitions
The utils code does not unwrap the element and still let it pass through up to the React reconcilier (which emits the warning)
Just adding this line fixes the issue
There was a problem hiding this comment.
Ah, great find! Let me try
Josh-Cena
changed the title
Josh-Cena
changed the title
|
OK I should have checked dev tools. I was overthinking it |
# Conflicts: # packages/docusaurus-theme-common/src/utils/admonitionUtils.tsx
slorber
added
backported
Pre-flight checklist
Motivation
This is the best fix I can think of—a
<template>element seems to be semantically the best fit here, since we are essentially having a "thunk" of HTML elements and we don't care what's in it, just need to transfer it elsewhere to be rendered.Test Plan
Test links
Deploy preview: https://deploy-preview-8246--docusaurus-2.netlify.app/tests/pages/markdownPageTests#admonitions
You would probably need to test this locally. Go to http://localhost:3000/tests/pages/markdownPageTests#admonitions, and see in the console that the warning (which can be reproduced on main) is gone.
Related issues/PRs