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

Does not parse emphesis like **bold** or *italic* #19

Closed
cougarten opened this issue Jul 5, 2017 · 10 comments · Fixed by #41
Closed

Does not parse emphesis like **bold** or *italic* #19

cougarten opened this issue Jul 5, 2017 · 10 comments · Fixed by #41

Comments

@cougarten
Copy link

example:
# My **bold** title

should render as:
<li>My <strong>bold</strong> title</li>

does render as:
<li>My **bold** title</li>

@martinlissmyr
Copy link
Collaborator

Hi. A PR with a fix would be very welcome. Although I would argue that no styling should be transfered to the TOC. The formatting should rather be stripped away.

@char0n
Copy link
Contributor

char0n commented Dec 1, 2017

format hook can be used to render this markup as html. But this creates additional problem how to get only text nodes from this rendered html...

@char0n
Copy link
Contributor

char0n commented Dec 1, 2017

Workaround:

const removeMarkdown = require('remove-markdown');
const toc = require('markdown-it-table-of-contents');
const md = require('markdown-it')();

md.use(toc, {
  format: removeMarkdown,  
});

const src = `
 [[toc]]
 # *Title*
 ## Subtitle 
`;

md.render(src, {});

@char0n
Copy link
Contributor

char0n commented Dec 1, 2017

I guess we can close this. Workaround/Solution mentioned above work for most usecases.

@martinlissmyr
Copy link
Collaborator

👍

@coryschires
Copy link
Contributor

Hi. Thanks for making this plugin! It's been really helpful.

I'd like to re-raise this issue. By default, I think this library should either:

  1. Strip out markdown (e.g. My **bold** title becomes My bold title).
  2. Process the markdown (e.g. My **bold** title becomes My <strong>bold</strong> title)

The current default behavior – rendering unprocessed markdown – is probably not what anyone wants or expects. I understand you can achieve either of these outcomes using a format function. But, ideally, users of this extension could avoid that work if a smarter default were in place.

Of the two options I've suggested, I think it would be better and easier to process the markdown. (Stripping could be tricky given the full range of possibilities, especially if you consider rarer options like superscript, subscript, etc.).

I'm using this lib to render ToCs for scientific articles. In my (admittedly complex) use case, headers may include a variety of formatting (e.g. italics, superscripts, even equations). And this formatting is often integral to header's legibility / meaning (i.e. it's not just a matter of style).

How to implement / Need help?

Looking at your code, I think the md object may have access to the underlying rendering functions. For example, I wrote a markdown-it extension which did something like:

this.env.md.renderInline("Some text which _may have formatting_")

And this allowed me to process markdown text within my extension – which I think is what we want in this case as well.

I'm not 100% sure how this would work in your code, but I think it's possible (and probably pretty easy). If you need help, lemme know and could probably make a PR.

@martinlissmyr
Copy link
Collaborator

martinlissmyr commented Aug 28, 2018

Hi! Thanks for chipping in...

The current default behavior – rendering unprocessed markdown – is probably not what anyone wants or expects.

I agree.

I'm using this lib to render ToCs for scientific articles. In my (admittedly complex) use case, headers may include a variety of formatting (e.g. italics, superscripts, even equations). And this formatting is often integral to header's legibility / meaning (i.e. it's not just a matter of style).

I haven't considered this scenario. It makes sense.

Of the two options I've suggested, I think it would be better and easier to process the markdown.

Given your examples I see your point. However, I think it would be preferable if it could be an option like so: parseMarkdownInHeadings: true or false where true should be the default option.

Do you think it's somewhat plausible to implement stripping markdown? Haven't looked at it in any depth but I imagine it's possible by either hooking into the markdown-it parser or by stripping markdown in the raw text via regexp or similar (I've seen libs that do that)...

@martinlissmyr martinlissmyr reopened this Aug 28, 2018
@coryschires
Copy link
Contributor

Thanks for considering this change! I like the idea of adding a parseMarkdownInHeadings option which defaults to true.

As I said, I think processing the markdown should be easy (maybe even a one-liner). Stripping the markdown is a bit trickier.

Here's what I would suggest:

1. Use an existing library

Both remove-markdown and strip-markdown seem decent. That said, they do seem a little greedy and could result in striping non-markdown text in rare edge cases (e.g. if your heading includes a non-markdown _ or [). But maybe that's okay?

2. Don't even try

Maybe, instead, give folks the option to render the raw markdown (i.e. parseMarkdownInHeadings: false) and let them deal with the complexity as well as the potential edge cases by writing a custom format function. For example, if I know my headers will only include *, then a simple regex would suffice. Or, if I have more possibilities, then I could consider using something like remove-markdown within my format function.

Most of all, I would avoid trying to write your own regex. Just my opinion but I think it could end up being a messy, half-solution. And, given a tricky problem, I'd rather avoid it altogether than apply a partial fix. But that's just my opinion.

@martinlissmyr
Copy link
Collaborator

martinlissmyr commented Sep 3, 2018

Ok, I hear you...

Then maybe we could just consider changing the default for the format option to a function that returns parsed markdown. Sort of what you initially suggested 😄 ...

A PR with this change would be very welcome!

@coryschires
Copy link
Contributor

@martinlissmyr Sorry I dropped the ball on this fix for several months. But, finally, I have a PR for you to review: #41

Thanks again for creating this extension!

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

Successfully merging a pull request may close this issue.

4 participants