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

[markdown] A way to prevent wrapping generated HTML with <p> tags when there is no need for them #101

Open
DmitrySharabin opened this issue Jun 30, 2021 · 5 comments

Comments

@DmitrySharabin
Copy link
Member

The Markdown plugin uses the Showdown library under the hood. And wrapping markdown in paragraph tags is the default behavior that might cause some issues and sometimes highly undesirable (e.g., see https://codepen.io/dmitrysharabin/pen/zYwxEPJ?editors=1100).

It is not a rare problem at all. There are issues concerning it in the Showdown repo as well:

As @tivie mentioned in their comment: Showdown does not support selective conversion (the ability to only parse and convert certain markdown elements), however, one can create an extension that removes disallowed “tags”.

So, we could add this code (with some modifications if needed) to our plugin:

showdown.extension("remove-extra-paragraphs", function () {
  return [{
    type: "output",
    filter: function (markdown) {
      return markdown.replace(/<\/?p[^>]*>/g, "");
    }
  }];
});

However, we need to decide whether we want this extension to be enabled plugin-wise or property-wise. If plugin-wise, how can we distinguish when to apply some extra transformations on the markdown and when don't? If property-wise, should authors add some extra attributes to the property, or maybe they should add a class?

Are there any other cases we should take into account?

@LeaVerou @karger What do you think?

@LeaVerou
Copy link
Member

I think this should be automatically enabled based on whether the property matches a given selector (which would include tag names that can't have paragraphs in them, but also a class as an escape hatch).

Another solution (which I suppose we don't want) is to use another Markdown converter. I recently used markdown-it which has a renderInline() method.

But do note that rendering inline is not just about removing paragraphs. We also don't want to allow block-level elements, like headings, code blocks etc.

@DmitrySharabin
Copy link
Member Author

But do note that rendering inline is not just about removing paragraphs. We also don't want to allow block-level elements, like headings, code blocks etc.

Exactly! It feels like writing our own parser. 😱

Another solution (which I suppose we don't want) is to use another Markdown converter. I recently used markdown-it which has a renderInline() method.

Well, we can give it a try in a separate plugin. And if it more suits our needs we can migrate to it later.

For now, I simply don't know which of these tasks is easier. 🤔

@LeaVerou
Copy link
Member

LeaVerou commented Jun 30, 2021

I wonder if we can just add a setting to our plugin about which parser to use instead of adding a whole new plugin. Then we can use inline rendering when it's available.

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Jun 30, 2021

Excellent point!

Like <el class="markdown" mv-markdown-converter="showdown">...</el>?

Or use the existing one: <el class="markdown" mv-markdown-options="converter: showdown">...</el>?

@LeaVerou
Copy link
Member

I think it would be much simpler to set the converter for the whole document, than on an element-by-element basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants