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

Breadcrumbs: restore Aria attribute and link #1068

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Jun 24, 2022

This reverts the changes to the breadcrumb partial done in #1011. In particular, this PR:

  • Reinstates the aria-current attribute
  • Reinstates the last breadcrumb as a link

In #1011, @at055612 notes that:

Making the leaf item not a link is possibly contentious but a quick google suggests this is an accepted practice. I think having a link to the page you are on is confusing and offers no value.

I agree on both points, and ( propose the following (which feels like a win-win): ensure that all breadcrumbs are links, but effectively disable the last breadcrumb. I use Bootstrap's disabled class and the Aria aria-disabled attribute. The documentation for aria-disabled explicitly mentions as a use case the situation of a link/button to a self.

Visually, you'll see no difference, e.g.:

image

Preview: for example see https://deploy-preview-1068--docsydocs.netlify.app/docs/get-started/docsy-as-module/

Note that while the last breadcrumb is disabled, its link can still be visited.

@LisaFC
Copy link
Collaborator

LisaFC commented Jun 24, 2022

LGTM!

@LisaFC LisaFC merged commit 1aa065f into google:main Jun 24, 2022
@chalin chalin deleted the chalin-im-fix-breadcrumbs-2022-06-24 branch June 24, 2022 09:19
@at055612
Copy link
Contributor

@chalin May be a bit late as this has already been merged, but does it make sense to have aria-current="page" for the leaf item on each of the the taxonomy results page cards (https://deploy-preview-1068--docsydocs.netlify.app/tags/labelling/).

Agree with the use of aria-disabled and disabled links, thanks for fixing that.

@chalin
Copy link
Collaborator Author

chalin commented Jun 24, 2022

As I mentioned elsewhere, I think that all ARIA attributes should be removed in that case. Let's move this discussion to #1071.

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 this pull request may close these issues.

None yet

3 participants