feat(mdxish): always compile callouts to JSX in mdxishMdastToMd#1498
feat(mdxish): always compile callouts to JSX in mdxishMdastToMd#1498eaglethrost wants to merge 16 commits into
Conversation
|
So this has a breaking change to the mdxish editor callout serializer in the monorepo? should we take over PR #17745 and make updates there and coordinate them landing at the same time? |
kevinports
left a comment
There was a problem hiding this comment.
Overall looks good! Just wanted to chat about the breaking change in the monorepo before we merge this.
Yep added the breaking change in the monorepo PR table serializer! https://github.com/readmeio/readme/pull/17745 |
kevinports
left a comment
There was a problem hiding this comment.
Code looks good to me.
Noticed one issue QAing locally with the monorepo linked. I'm not seeing some of the font-awesome icon types rendering correctly:
CleanShot.2026-06-04.at.13.48.41.mp4
We may need to make similar changes to my work here https://github.com/readmeio/markdown/pull/1492/changes to support different icon types?
Had a look at this, it seems to only affect "far" icons (fad & others are fine), which are all of the icons in the Icon section in the emoji picker. It looks like this issue comes from the Callout component style itself, particularly: It seems to not have caught on the far icon data, we can fix it by adding a fallback variable like But actually using your changes in #1492 works as well, we need to reuse the Icon component in the Callout & that will render the far icons fine. Since we're going to merge that, I'll merge that over here as well & reuse the Icon component. Demo of it working in monorepo, before the "Icons" emoji would not render, but the duotones would: Screen.Recording.2026-06-05.at.7.04.25.pm.mov |
…e-callout-compiler-to-jsx
There was a problem hiding this comment.
The Callout now renders icons through the Icon component, which emits FA icons as <i> (instead of <span class="callout-icon_fa">), so we need to retarget the icon style to i/span and scope the ::before glyph to span only, which prevents the legacy icon-font glyph from double-stacking on top of Font Awesome's own glyph
kevinports
left a comment
There was a problem hiding this comment.
Sorry one thing just occurred to me for the Icon component --
Some users may target the existing icon markup for custom styles, and these changes could potentially break any custom css targeting these nodes.
Can you take a look at each of the components consuming the Icon and ensure that the DOM and classnames for existing FA icons are retained, and we only add support for emojis and different FA icon styles in an additive way?
6d7654e to
961feaa
Compare
961feaa to
6d7654e
Compare
Yep, regarding this there'll be a bit more issue with callouts because before it was using However, using |
| <span className={`callout-icon callout-icon_fa ${icon}`} /> | ||
| ) | ||
| ) : null} | ||
| {icon ? <Icon className="callout-icon" faClassName="callout-icon_fa" icon={icon} /> : null} |
There was a problem hiding this comment.
Since Icon uses <i> to render font awesome icon, now it'll no longer use <span>, which is a breaking change if there's custom CSS that specifically targets <span> + callout icon classes. But this is a necessary fix to make font awesome icon renderable in Callouts, currently it doesn't render at all.
4a8d854 to
921630d
Compare
921630d to
4a8d854
Compare
Note
This PR is blocking readme PR https://github.com/readmeio/readme/pull/17745 & should be markdown bumped in that PR since it contains breaking change
🎯 What does this PR do?
Updates the mdxish callout serializer (
mdxishMdastToMd) to always compile callouts to<Callout>JSX syntax instead of the legacy MD blockquote format.PR #17745 adds icon + color selection for callouts in the MdxishEditor UI, but the engine always serialized callouts to the legacy blockquote (
> 📘 Hey), which dropped any custom icon/theme on round-trip. This PR persists those selections.processor/transform/mdxish/callout-to-jsx.ts— a transform that converts everyrdme-calloutmdast node into anmdxJsxFlowElement(<Callout icon=… theme=…>), reusingtoAttributesand themdxJsxStringifyextension already wired intomdxishMdastToMd.mdxishMdastToMd(alongsidemdxishTablesToJsx). The RMDX (mdx) pipeline and the sharedprocessor/compile/callout.tsblockquote compiler are untouched, so RMDX behavior is unchanged.Also in order for the Callout to render the
faricons, we need to adjust the Icon rendering such that it can properly apply them, so we bring over the work from #1492 where we create a centralised Icon component to support those icons.Note
But the Icon adjustment is a breaking change if there's custom CSS that specifically targets
<span>+ callout icon classes, since Icon uses<i>to render font awesome icons instead of<span>. Though I think this is a necessary tradeoff and people should be aware of it.🧪 QA tips
Note
Test this with the readme PR https://github.com/readmeio/readme/pull/17745 which adds an emoji picker & allows adding font awesome icons to callout
> 🚧 It works!→<Callout icon="🚧" theme="warn">### It works!…</Callout>, body-only callouts re-deriveempty: true, and authored JSX callouts round-trip unchanged in shape.📸 Screenshot or Loom
Before:
Screen.Recording.2026-06-01.at.6.54.56.pm.mov
After (with the callout editor serializer changed locally):
Screen.Recording.2026-06-01.at.6.56.27.pm.mov
Testing with the callout icon menu change (with the callout editor serializer changed locally):
Screen.Recording.2026-06-01.at.7.05.41.pm.mov