search

valeriangalliat / markdown-it-anchor

A markdown-it plugin that adds an `id` attribute to headings and optionally permalinks.
The Unlicense
291 stars 72 forks source link

Fix level selection logic #90

Closed Maximization closed 3 years ago

Maximization commented 3 years ago

When I read the docs on the level option:

Minimum level to apply anchors

I assumed, if I set level: 3 then h1, h2 & h3 will get anchored. Current behaviour however is h3, h4, h5 & h6 get anchors.

I think the confusion stems from higher heading = lower number (h1) and lower header = higher number (h6). It also doesn't make sense to set anchors on h3 through h6 and not on h1 and h2.

This bug was introduced in this commit from 4 years ago. Before that, the logic was in fact selection >= level instead of what is now: level >= selection. This PR fixes the bug by reversing the math operand: level <= selection.

valeriangalliat commented 3 years ago

Thanks for flagging this!

I think it's a bit more complex than that; even in the early versions the behaviour of level was to set a minimum level as per the number in the h tag.

The reason I added this feature was because I didn't want my h1 to have an anchor next to them, I would just link to the page itself without an anchor in that case).

Reversing that behaviour would be breaking, but I see how one would want to prevent adding anchors to (visually) smaller headers as well.

Maybe having minLevel and maxLevel options would make that more clear. What do you think?

Then I would keep the current behaviour of level as is to maintain backwards compatibility, but probably remove it from the documentation altogether; if we have minLevel and maxLevel I don't think anyone would want to pass an array of levels that are not contiguous.

If this sounds like a good solution to you, let me know if you want to implement it in this PR, otherwise I'll take care of it. :)

Cheers!

Maximization commented 3 years ago

Oh I see. Perhaps the docs are clear and I misread, in which case you can ignore this. I don't have h1 in markdown files, only in .njk templates, so didn't consider the reverse use case but makes sense. I'm using level: [2, 3, 4] now which works well.

Not sure how much value minLevel and maxLevel add over the existing array config option for level. Personally I'm fine with using what we have today. It's flexible and keeps a small API footprint.

Thanks for answering and appreciate your input!