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

Blank <p> added between "Tweet" embeds #65

Closed
mattrossman opened this issue Oct 20, 2023 · 2 comments · Fixed by #66
Closed

Blank <p> added between "Tweet" embeds #65

mattrossman opened this issue Oct 20, 2023 · 2 comments · Fixed by #66
Labels
bug Something isn't working

Comments

@mattrossman
Copy link
Contributor

mattrossman commented Oct 20, 2023

When multiple Twitter links appear in an MDX file, there is a blank <p> tag between them, which can create awkward gaps on the page when markdown is styled.

Minimal Reproduction:

https://codesandbox.io/p/sandbox/astro-embed-tweets-extra-p-issue-4z4zq3?file=%2Fsrc%2Fpages%2Findex.mdx%3A1%2C1

Notice how there is a large gap between the two tweets, but the gap is not present between YouTube embeds.

2x Tweets markup (each item is prefaced by a blank <p><astro-embed-tweet></p> and followed by a blank <p></p>:

CleanShot 2023-10-20 at 17 34 10@2x

2x YouTube markup (each item is in a single <p>:

CleanShot 2023-10-20 at 17 32 44@2x

I would like the Tweet embed to behave similar to the YouTube one, so it just lives under a single element for consistent spacing.

@mattrossman
Copy link
Contributor Author

mattrossman commented Oct 20, 2023

Narrowed down the issue to the usage of set:html in Tweet.astro. It produces a single element without this directive, but once the directive is added it bleeds other elements outside of the root <p>

It seems to be sensitive to the kinds of elements present in the inner HTML.

For instance this (using <span>):

<Fragment set:html={`<astro-embed-tweet><span>test</span></astro-embed-tweet`} />

Produces the markup you'd expect:

<p>
  <astro-embed-tweet>
    <span>test</span>
  </astro-embed-tweet>
</p>

But this (using <div>):

<Fragment set:html={`<astro-embed-tweet><div>test</div></astro-embed-tweet`} />

Produces this with the <div> in a different spot in the markup.

<p>
  <astro-embed-tweet></astro-embed-tweet>
</p>
<div>test</div>

@delucis
Copy link
Owner

delucis commented Oct 21, 2023

Thanks for the report and the investigation! This is actually an issue with HTML parsing. If you look at the HTML generated, it looks like this:

<p>
  <astro-embed-tweet>
    <blockquote class="twitter-tweet" data-dnt="true">
      <p lang="en" dir="ltr">Tweet</p>
      &mdash; Username (@handle) <a href="...">Month D, YYYY</a>
    </blockquote>
  </astro-embed-tweet>
</p>

The problem is that <blockquote> is not permitted inside <p>, so when the browser parses this, it does something like:

  1. <p> — OK, starting a paragraph

  2. <astro-embed-tweet> — custom element, adding it inside our paragraph

  3. <blockquote> — uh oh, they forgot to close the <p>, no problem, we can do that for them (and close any children too) and then keep going.

So the result is:

<p>
  <astro-embed-tweet>
  </astro-embed-tweet>
</p>
<blockquote class="twitter-tweet" data-dnt="true">
  <p lang="en" dir="ltr">Tweet</p>
  &mdash; Username (@handle) <a href="...">Month D, YYYY</a>
</blockquote>

Whether or not you get the large gaps, depends on your CSS (in your reproduction display: grid on the <section> containing the embeds), so I missed this in the test site. This problem impacts https://astro-embed.netlify.app/integration/ for example, but there’s no custom CSS so there’s no visual impact.

I’ll note this is only an issue with the embeds() MDX integration, if you use the component directly, there’s no extra <p>:

import { Tweet } from 'astro-embed'

<Tweet id="https://twitter.com/astrodotbuild/status/1511750228428435457" />

We can probably fix the embeds() remark plugin to remove the parent <p> tag in these cases to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants