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

Remove ARIA attributes from taxonomy lists #1072

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Jun 24, 2022

I've chosen to avoid complicating the logic of the breadcrumbs partial since it's complicated enough already. This PR implements what I proposed earlier: it strips out unnecessary classes and irrelevant ARIA labels from the "breadcrumbs" used in the taxonomy lists.

You'll notice that I've removed the disabled state from the last breadcrumb since it no longer makes sense to disable it (because we're not on that page anymore :)).

Preview: https://deploy-preview-1072--docsydocs.netlify.app/categories/taxonomies/


Screenshots

Before:

image

After:

image

/cc @at055612

@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from 3a633dc to 11dd63d Compare June 24, 2022 12:45
@chalin
Copy link
Collaborator Author

chalin commented Jun 24, 2022

@at055612 - PTAL and approve if this solution is suitable for you.

@at055612
Copy link
Contributor

@chalin Agree with making the leaf link enabled on the taxonomy results.
I think a comment would be useful to explain why the regex replace is there.
I did play with passing a flag into the partial to define whether it was a page breadcrumb or not and thus whether to include the aria bits, but it wasn't working for some reason. My only worry with the regex approach is that if someone changes the implementation of the partial they may unknowingly break the the taxonomy results.

I did wander about keeping the aria-label on the <nav> but setting its value to "breadcrumb for {{ .LinkTitle }}". However I think aria-label should be unique and we can't guarantee that. Setting it to something like "breadcrumb for result N" would be an option but the partial has no knowledge of its index.

@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from 11dd63d to bd5260a Compare June 24, 2022 14:54
@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from 5c6d19c to 15bb6e0 Compare July 4, 2022 15:52
@LisaFC
Copy link
Collaborator

LisaFC commented Jul 4, 2022

LGTM, though agree with @at055612 that a comment for the regex replace would be useful, especially for future users who might want to modify or override the partial.

@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from 97b376b to 08130c0 Compare August 1, 2022 15:18
@chalin
Copy link
Collaborator Author

chalin commented Aug 1, 2022

Added a comment and rebased. PTAL

@chalin chalin requested a review from LisaFC August 1, 2022 15:25
@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from 08130c0 to ff62fd4 Compare August 9, 2022 14:46
@chalin
Copy link
Collaborator Author

chalin commented Aug 9, 2022

Rebased. Pinging @LisaFC and @geriom for a review.

@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from ff62fd4 to bd70b0d Compare August 15, 2022 07:21
@chalin
Copy link
Collaborator Author

chalin commented Aug 15, 2022

@LisaFC - just a reminder that this is ready for your review.

@chalin chalin force-pushed the chalin-im-tax-bc-2022-06-24 branch from bd70b0d to 8bda2ca Compare August 15, 2022 16:29
@chalin chalin merged commit 0c530c3 into google:main Aug 15, 2022
@chalin chalin deleted the chalin-im-tax-bc-2022-06-24 branch August 15, 2022 17:00
fekete-robert pushed a commit to fekete-robert/docsy that referenced this pull request Sep 13, 2022
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.

Remove ARIA attributes from embedded-page breadcrumbs in taxonomy lists
3 participants