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

fix (a11y): breadcrumb a11y issues on blog page #805

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

virus-rpi
Copy link
Collaborator

@virus-rpi virus-rpi commented Sep 2, 2024

Chages:

  • change breadcrumb text color from rgb(0, 0, 0, 0.5) to rgb(0, 0, 0, 0.65) to make color contrast WCAG AA compliant
  • add aria-label to breadcrumb container
  • wrap breadcrumb container with nav landmark
  • replace overflow: hidden with overflow-x: clip to only hide the overflow in the x direction
  • replace padding: 0 with padding-left: 4px so the outline isn't cut of at the left side
  • add transform: translateX(-4px) to breadcrumb container to counteract the visual changes through the padding change
  • add display: inline-block to the breadcrumb link to fix the shape of the focus outline

Before:
image
After (without display: inline-block added ):
image

… 0, 0.65) to make color contrast WCAG AA compliant

fix: add aria-label and roll to breadcrumb container
fix: remove overflow: hidden from BreadcrumbContainer to prevent focus outline cutoff
@virus-rpi virus-rpi requested a review from a team as a code owner September 2, 2024 14:48
Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for satellytes ready!

Name Link
🔨 Latest commit 08c54d2
🔍 Latest deploy log https://app.netlify.com/sites/satellytes/deploys/66ec41b7bb64bd0008d53de5
😎 Deploy Preview https://deploy-preview-805--satellytes.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 86 (🟢 up 3 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@virus-rpi virus-rpi mentioned this pull request Sep 2, 2024
20 tasks
@virus-rpi virus-rpi changed the title Fix: breadcrumb a11y issues on blog page fix (a11y): breadcrumb a11y issues on blog page Sep 2, 2024
Copy link
Member

@georgiee georgiee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for that.
It would help if you can describe the effects in addition to the changes because a11y is pretty abstract for many folks. Even with some experience I've issues following the actual motivation for your changes

I found two issues while the color change is good.

  1. I can see the difference with the colour and I understand the contrast thing. Fine!

CleanShot 2024-09-04 at 09 40 04

  1. I do not understand the role "navigation" because that seems to remove the list + listitem role already given by the ol tag. I think a better alternative would be to just wrap the ol with an additional nav tag. That way people can use the semantics of the list (screen reader more easily navigate lists) plus the nav semantic, that a screen reader may use to tell about the significant navigation options on the page

CleanShot 2024-09-04 at 09 41 25

Ref:

  1. I don't see any difference of the focus outline. Can you demonstrate? I also wonder why removing the overflow has no other impact. Why do we have it in the first place anyway?

You see that I have feedback to all three changes. I have the vague feeling that 3 very small PRs would have been much better here as you could focus on documenting and demonstrating the purpose of the change. On the other hand, it's quite an overhead. Let's go with this PR, fix the issues and then continue.

@virus-rpi virus-rpi requested a review from georgiee September 19, 2024 15:28
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.

3 participants