wooorm / xdm

Just a *really* good MDX compiler. No runtime. With esbuild, Rollup, and webpack plugins
http://wooorm.com/xdm/
MIT License
595 stars 18 forks source link

Unexpected <p>'s #47

Closed peduarte closed 3 years ago

peduarte commented 3 years ago

Hi Titus, thanks for xdm and for all the work you've done and continue to do in this space.

I build and maintain all the marketing and documentation sites for Modulz, Stitches and Radix

Recently I've upgraded nearly all of them from next-mdx-enhanced to next-mdx-remote but I've come across mdx-bundler by Kent (powered by xdm) and have decided to give it a go for the Stitches docs

Overall the migration was painless, but there's one particular issue that I'd like to raise.

In some of our documentation pages, we have a Preview component, where we render a React Component as an example of the code block we're demoing, like so:

image

The mdx responsive for that preview is as follow:

<Preview>
  <div style={{ display: 'flex', gap: '20px', alignItems: 'center' }}>
    <DemoButton color="gray" size="small">
      Button
    </DemoButton>
    <DemoButton color="violet" size="small">
      Button
    </DemoButton>
    <DemoButton color="gray" size="large">
      Button
    </DemoButton>
    <DemoButton color="violet" size="large">
      Button
    </DemoButton>
  </div>
</Preview>

But the above, generated this:

<div data-preview="true">
  <div style="display: flex; gap: 20px; align-items: center">
    <button>
      <p>Button</p>
    </button>
    <button>
      <p>Button</p>
    </button>
    <button>
      <p>Button</p>
    </button>
    <button>
      <p>Button</p>
    </button>
  </div>
</div>

Notice how each the text node in button is wrapped in p.

I've read #42 and #38 which were somewhat related, and based on your suggestions there, I was able to "solve this problem" in the following ways:

Close button on same line

<DemoButton color="gray" size="small">Button</DemoButton>

This works, but whenever I save prettier formats the code and breaks it again 🙄 Disabling prettier for mdx is not an option for us, unfortunately.

Wrap Preview in {}

{<Preview>
  <div style={{ display: 'flex', gap: '20px', alignItems: 'center' }}>
    <DemoButton color="gray" size="small">Button</DemoButton>
    <DemoButton color="violet" size="small">Button</DemoButton>
    <DemoButton color="gray" size="large">Button</DemoButton>
    <DemoButton color="violet" size="large">Button</DemoButton>
  </div>
</Preview>}

This works, but Im worried its "unconventional". It seems that it requires our team to learn some new syntax, and I can see things going wrong as our team and documentation sites grow. But maybe I should embrace that this is more like MDX-like, and not "mdx"? I dunno.


I'd like to ask, from your perspective, does it make sense that the text is wrapped in paragraphs? Am I limited to the solutions above?

Thanks again!

wooorm commented 3 years ago

Thanks for such a detailed issue! ✨ #38 and #42 do describe most of my feelings here.

Close button on same line

I think this is an issue in Prettier, which is now based on MDX@1 — it hasn’t updated for the format of the latest mdx-js/mdx beta (also supported here).

Wrap Preview in {}

I’d say this is a good solution. Also not something in MDX@1. And: in braces you don‘t need to tags-on-one-line — they can be however you want to format JSX. I think this is a pretty good way to go.


One alternative: these code examples seem to have input and output. Maybe they can be wrapped in code instead? And then have a component that turns them into rendered JSX + JSX code or whatever? That could also be done as a plugin.


Also, “semantically”, these buttons probably should be grouped in a paragraph?


from your perspective, does it make sense that the text is wrapped in paragraphs?

It mostly makes sense from my perspective. But it’s not perfect. We received a lot of feedback that the standard markdown rules for HTML were definitely not expected. Hence an alternative. I hope this is closer. But for an alternative you need new rules. I think these rules are okay, but I’m definitely monitoring what users feel.

peduarte commented 3 years ago

Thanks for that, appreciate it 👍

I'm gonna go with wrapping JSX in {} as a rule-of-thumb, and see how it goes for our team.

I like your idea of wrapping them in code, will have a play with this and see how it feels.

But you gave me another idea, which is I rarely need to set the text node on the button, so I might as well bundle children in the DemoButton so I can use it as a self-closing element.

Since at this point in time this is behaving by design, I'll close this issue.

Thanks!