search

erusev / parsedown

Better Markdown Parser in PHP
https://parsedown.org
MIT License
14.69k stars 1.12k forks source link

Create ID's for Header elements so they can be referenced in anchor tags #765

Open netniV opened 4 years ago

netniV commented 4 years ago

This allows rendered markdown to be compatible when viewed via parsed markdown or GitHub

Closes #710

Ayesh commented 4 years ago

I was reading how league/commonmark does the slugs, and it turns out to be stripping a lot characters as well, so I sent a PR https://github.com/thephpleague/commonmark/pull/467

netniV commented 4 years ago

I was reading how php-league/commonmark does the slugs, and it turns out to be stripping a lot characters as well, so I sent a PR thephpleague/commonmark#467

Didn't even know about that one 👍

netniV commented 4 years ago

so the tests are failing now because the leading # is no longer applicable. The question is, should we trim the leading hyphen?

aidantwoods commented 4 years ago

Hi @netniV and @Ayesh thanks for the great work you've done towards these changes to arrive at a very sensible slug function :)

The topic of slugs (or header ID's) have come up a few times in the past (e.g.), and the position has previously been that there wasn't a universal choice that everyone using the library would want, so to avoid making the choice for everyone we'd leave it alone and up to extensions.

I've built upon the changes you've worked on here in PR #767, which modifies these changes for the new 2.0.x branch, as well as adding the option for a user-defined slug function (so that anyone who doesn't like the default can specify their own).

@netniV I've listed you as a co-author in the commit to that PR (which I hope is okay?) because most of the work has been done here in arriving at a sensible slug function to use.

netniV commented 4 years ago

Looks good. I added one comment as per our discussions here over str_replace. More involved is your change but I was thinking it should be an optional addition as it could affect other elements due to a duplication of ID

The only other thought is with trimming a trailing and leading hyphen as that seems wrong to keep those to me.

netniV commented 4 years ago

Does this mean this PR should now be closed as it will only be applied to the 2.0 version?

cpeel commented 3 years ago

@aidantwoods - is there a timeframe for when 2.x will be released? I'd rather have my users use the auto IDs for headers than enable ParsedownExtra.

netniV commented 3 years ago

It would be nice if this PR could be merged into the 1.8.x branch and a 1.8.1 released. Either that, or have 2.0 released. At the moment, I can't get these changes through composer without publishing my own version of the package.

Ayesh commented 3 years ago

I don't think drastic changes like this would be realistically merged to the 1.x branch. What I do is simply extend the Parsedown class, and override the necessary parts. It turned out a good approach long term because I wanted to add some extra markup on my own too.

netniV commented 3 years ago

Let me see if I can manage that. I did something for images so it may be possible.

Personally, I wouldn't call it a drastic change, but I can see ID conflicts could break things so I see where you are coming from. I would submit a forked version with it in, but that seems a waste for such a small feature.

netniV commented 3 years ago

Firstly, I think it would be good to have this 1.8.0-beta7 as a full release given there has been no other changes in the past year that I can see (Aug 2020). Requiring a beta version in a composer.json looks ... well bad to me. The reason that I need to use the beta version is for handler array usage.

Now that I have fixed on the latest versions, I am still getting a problem which is due to changes in this beta version causing an issue with your parsedown-extra class.

https://github.com/netniV/ParsedownID/issues/1

netniV commented 3 years ago

@Ayesh or @aidantwoods do you have the ability to sort out the current mess of Parsedown vs Parsedown-Extra ? Due to the beta changes that haven't been published as a version, parsedown-extra relies on older versions. But to use the newer array-based handler definitions, you need this beta version and parsedown-extra apparently hasn't been updated in two years so can't handle it.

aidantwoods commented 2 years ago

It's been a while, but now very close to a finished product for 2.0 in Parsedown (https://github.com/erusev/parsedown/pull/708) and ParsedownExtra (https://github.com/erusev/parsedown-extra/pull/172). I'd appreciate any feedback you have :)

netniV commented 2 years ago

I will try and see if I have time. Things are rather busy at the moment though.