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

Enhancement: add options to hide the link altogether, and set the ID on the heading instead of a separate <a> #21

Closed
uandco opened this issue Dec 1, 2022 · 10 comments
Assignees
Labels

Comments

@uandco
Copy link

uandco commented Dec 1, 2022

Having a link in each heading, even with empty text, creates unnecessary tabbing step when navigating with the keyboard.
It would be great to have an option that disables the <a class="anchor"> element altogether.

Another option to set the id attribute on the heading itself would be great as having an extra element before the heading can cause styling issues, for example when using :first-child. I know I could use a specific class on the anchor and change to h2:first-child, a.specific-class:first-child + h2 but that's a massive technical debt while the id attribute should have been on the h2 to start with.

There might be other accessibility considerations to take into account to improve the plugin, this is a good read.

@uandco uandco added the bug label Dec 1, 2022
@i-just i-just assigned i-just and unassigned i-just Dec 13, 2022
@i-just i-just self-assigned this Dec 16, 2022
@brandonkelly
Copy link
Member

Thanks! Anchors 2.4.0 (Craft 3) and 3.1.0 (Craft 4) have been released with a fix for this.

@uandco
Copy link
Author

uandco commented Jan 16, 2023

Thanks @brandonkelly, I upgraded to 3.2.0 but the anchor element is still there, even tho my config is:

<?php
return [
	'anchorLinkText' => ''
];

I couldn't see any new parameter in the documentation, to remove the anchor link altogether. Am I missing something?

Also the id attribute is still set to an element before <h2>, not the <h2> itself, even though it's a <span> now whereas it was <a> before if I remember correctly?

I really think the documentation is missing the new configuration options to change the behaviours I was describing here?

@uandco
Copy link
Author

uandco commented Jan 16, 2023

Sorry I had a look at the PR and none of these changes address the 2 issues I mentioned. The id attribute should always be set on the heading element itself, then an extra option should be offered to hide the anchor link (typical use case, we just need the anchor to link from another page, but we don't need to tell the visitors every heading has an anchor they can copy).

The expected output when disabling the anchor link would be:
<h2 id="microbiological-criteria">Microbiological criteria </h2>

At the moment, I can use display: none on a.anchor as a workaround to not provide the links at all, but the id being on a span is still an issue.

@i-just
Copy link
Contributor

i-just commented Jan 16, 2023

Hi, accessibility improvement has been actioned (the unnecessary tabbing step was removed). That’s why we now have the span tag, and you are correct, it was an a before.

If you set the anchorLinkText to an empty string, the a tag will still be there; it’s just that the text that’s displayed for that tag will be an empty string. That’s expected behaviour. To be clear, no new configuration has been added.

I don’t believe we can always add the ID to the heading tag itself because what if that heading already has an ID?

Please note that you can also tap into the [plugin api] (https://github.com/craftcms/anchors#plugin-api) to enhance the functionality to match your specific needs.

@uandco
Copy link
Author

uandco commented Jan 16, 2023

Well, if the heading already has an id, you don’t have to generate one and you can use it in the anchor link ;-)

@i-just
Copy link
Contributor

i-just commented Jan 16, 2023

Doh, apparently, I like to overcomplicate things...
Let me think about it some more, and I’ll let you know if we will implement this.

@uandco
Copy link
Author

uandco commented Jan 16, 2023

All good @i-just , thanks for looking into this!

@i-just
Copy link
Contributor

i-just commented Jan 17, 2023

Hi, @uandco; we discussed this internally and decided that not rendering the anchor at all is out of scope for this plugin. As mentioned before, you can have a look at leveraging this: https://github.com/craftcms/anchors#plugin-api to achieve what you’re after.

In terms of setting the id on the heading directly, it used to work this way until version 1.3. I’m not 100% sure why it was changed, but it could have been to help with the styling of the scrolling behaviour. Since the introduction of scroll-padding-top, this is no longer as big of a concern, so we’ve decided to make it possible to drop the additional span tag with the id that was used to anchor to and move that id to the heading tag. By default, you will still get the additional span tag, but you’ll be able to opt into the new behaviour (having the id directly on the heading) via a new config option.

@uandco
Copy link
Author

uandco commented Jan 17, 2023

Thanks @i-just , the anchor is not a big deal, there's CSS workarounds to hide it altogether or only show it to screen readers, etc. Adding a new config option to set the id to the heading sounds like a great idea for those making use of the extra element and remain backward compatible.

@uandco
Copy link
Author

uandco commented Jul 5, 2023

@i-just I just saw the 3.3.0 update, thanks for adding that new useAdditionalTagToAnchorTo option!

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

No branches or pull requests

3 participants