Closed kleinfreund closed 2 years ago
I started to play around with the project a bit to figure out if I’m able to contribute a change that would allow me to cover my use case a bit more gracefully (of course at this point, I don’t even know if what I’ve assembled in words above is even sensible, so this is very much just on my own initiative).
While doing that, I had a look at the tests to familiarize myself with some usage scenarios and I wondered if you would be open to accepting a pull request for migrating the existing test suite to Jest so that they can be structured a bit more neatly using describe
and test
blocks. I’m happy to submit a PR for that.
Hey! That's a cool way of rendering permalinks, I didn't consider that in the previous version and it definitely should be possible without a custom renderer.
It's also true that the readme is lacking about how to make custom renderers, I'll update it to cover that.
I'm thinking of allowing the ariaHidden
permalink (which is essentially the legacy one with a different way to pass options and the addition of aria-hidden="true"
) to not add ariaHidden
(so I need a different name for that, maybe linkInsideHeader
with an ariaHidden
option).
That way you could do:
const markdownItAnchorOptions = {
permalink: anchor.permalink.linkInsideHeader({
symbol: `
<span class="header-anchor__icon">
<span aria-label="Link Symbol" role="img">🔗</span>
<span class="visually-hidden">Jump to heading</span>
</span>
`.trim(),
placement: 'before'
}
};
As for the tests, while bare assert
was fine in the early stages of this project, it's true it grew quite a bit and could use describe
and test
blocks. I think I would keep I took this as an opportunity to try AVA which assert
as the assertion library and just use Mocha to provide describe
/test
, so the task would consist in organizing the existing tests in functions and giving them descriptions. Let me know if you still want to contribute that. :)test.js
now uses!
Okay linkInsideHeader
is available with version 8.2.0 that I just released, that should be enough to make your use case work with the markup above!
I think it's worth mentioning that one of the reasons the original permalink was deprecated is because it was challenging to make it accessible. Even with the aria-label/visually-hidden markup, you can notice that by listing the page headings and links in VoiceOver (Caps Lock + U). For headers it reads:
And when requesting the links in the page:
On top of that aria-label
might not be reliably picked up by translation services so it's often recommended to avoid it for this reason. The other permalink option were designed to integrate even better with screen readers, and I think they're worth taking a look at.
For example linkAfterHeader
will read "Title 1, Title 2, Title 3" for headings and "Permalink to “Title 1”, Permalink to “Title 2”, Permalink to “Title 3”" for links, which is arguably more useful, but that markup do require styling updates so it's not an instant fix.
I hope this help, cheers!
Hey there,
thanks for the thorough and helpful response. I’ve just updated the site where I’m using the aforementioned style using the follow configuration (i.e. exactly what you suggested):
const markdownItAnchorOptions = {
permalink: markdownItAnchor.default.permalink.linkInsideHeader({
symbol: `
<span class="header-anchor__icon">
<span aria-label="Link Symbol" role="img">🔗</span>
<span class="visually-hidden">Jump to heading</span>
</span>
`.trim(),
placement: 'before',
}),
};
I have read the conversations feeding into the decisions made regarding the deprecation of permalinks and understand the motivation and results, too. I’ll look into improving the markup on my site separately (i.e. separately from the dependency update). I hope translation services like that built into Chrome eventually pick up on the likes of aria-label
to make developer choices more flexible.
I’ll close this issue for now because I for one am satisfied with the outcome.
With the changes introduced in v8.0.0, a couple of preset permalink styles are available (see https://github.com/valeriangalliat/markdown-it-anchor#permalinks). I tried migrating my use of markdown-it-anchor to these styles to remove the deprecation warning from my build output.
Unfortunately, it doesn’t seem possible to reproduce my header anchor markup with any of the options:
The options on the provided presets only partially fit my needs:
Requirement 1: Anchor is within the heading element.
I cannot use
linkAfterHeader
because it renders the anchor as a sibling rather than a child elment. That’s fine in principle. After all, that’s what this style is meant to do.Requirement 2: Customize anchor markup or at least set the class name and visually-hidden class name, perhaps customize the anchor text based on the heading text.
I cannot use
headerLink
because these customizations aren’t possible. According to the documentation, it accepts a few default options, but I believe that rather means it has asome default options because the type definitions don’t list any parameters.Requirement 3: Anchor is exposed to screen readers.
I cannot use
ariaHidden
because it appliesaria-hidden="true"
To summarize, the configurability of
linkAfterHeader
works well for me were it not for the markup ending up as a sibling element of the heading rather than a child.I wonder why the flexible configurability is not available on all presets. More specifically, I don’t think it’s ultimately very sensible to tie the following two aspects strictly together based on which preset one chooses:
In my case, the appropriate solution is probably to use a custom render function. That’s probably not a big deal in practice, but I don’t know how to go about this because that feature is not documented as far as I can tell.
I would prefer that the various permalink styles all support the same degree of customization. Especially the position of the anchor is something I’d like to generally be able to customize regardless of style preset. A good analogy to what I thought I’d be able to do is the
Element.insertAdjacentHTML
API, in particular itsposition
parameter:There’s a good chance I’m missing a key aspect of the picture here, but right now, I’m just a bit lost as to what option’s best suited for me going forward. For now, I will keep using the following set of options and hope they don’t actually get removed:
On a slightly related note, I would appreciate if deprecations are explicitly marked in the changelog. The changelog entry for version v8.0.0 looks very harmless and appears as a mere feature addition and while my current configuration still works, should it get removed (which I assume is the case since it warns me in the log output that the option is deprecated), I would need to migrate to a set of options that I currently don’t understand well (because implementing a custom permalink renderer is undocumented).