-
Notifications
You must be signed in to change notification settings - Fork 106
fix(jump-links): apply aria-label to list #1882
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
Conversation
🦋 Changeset detectedLatest commit: 96206e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
f7d79dd to
d5deaef
Compare
|
Hey @bennypowers, upon initial review the code looks good! I will test with a screen reader shortly and report back. |
|
✔️ Deploy Preview for patternfly-elements ready! 🔨 Explore the source changes: 96206e7 🔍 Inspect the deploy log: https://app.netlify.com/sites/patternfly-elements/deploys/622910b4a0e82e000937f95d 😎 Browse the preview: https://deploy-preview-1882--patternfly-elements.netlify.app |
d9469f6 to
ce2db8d
Compare
🏮 Lighthouse report for the changes in this PR:🔴 🟢 🟢 🟠 ⚪️ https://deploy-preview-1882--patternfly-elements.netlify.app/kitchen-sink/🔦 Lighthouse Report for https://deploy-preview-1882--patternfly-elements.netlify.app/kitchen-sink/
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Styles🔦 Lighthouse Report for Styles
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Accordion🔦 Lighthouse Report for Accordion
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Autocomplete🔦 Lighthouse Report for Autocomplete
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Avatar🔦 Lighthouse Report for Avatar
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Badge🔦 Lighthouse Report for Badge
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Band🔦 Lighthouse Report for Band
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Button🔦 Lighthouse Report for Button
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Card🔦 Lighthouse Report for Card
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Clipboard🔦 Lighthouse Report for Clipboard
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Codeblock🔦 Lighthouse Report for Codeblock
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Collapse🔦 Lighthouse Report for Collapse
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Cta🔦 Lighthouse Report for Cta
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Datetime🔦 Lighthouse Report for Datetime
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Dropdown🔦 Lighthouse Report for Dropdown
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Health Index🔦 Lighthouse Report for Health Index
💥 Assertion Failures
🔴 🟢 🟠 🟢 ⚪️ Icon🔦 Lighthouse Report for Icon
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Icon Panel🔦 Lighthouse Report for Icon Panel
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Jump Links🔦 Lighthouse Report for Jump Links
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Markdown🔦 Lighthouse Report for Markdown
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Modal🔦 Lighthouse Report for Modal
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Number🔦 Lighthouse Report for Number
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Page Status🔦 Lighthouse Report for Page Status
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Primary Detail🔦 Lighthouse Report for Primary Detail
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Progress Indicator🔦 Lighthouse Report for Progress Indicator
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Progress Steps🔦 Lighthouse Report for Progress Steps
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Readtime🔦 Lighthouse Report for Readtime
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Select🔦 Lighthouse Report for Select
💥 Assertion Failures
🟠 🟢 🟢 🟢 ⚪️ Tabs🔦 Lighthouse Report for Tabs
💥 Assertion Failures
🟢 🟢 🟢 🟢 ⚪️ Toast🔦 Lighthouse Report for Toast
💥 Assertion Failures
|
zeroedin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me as far as the code changes. I tested a bit with mac voiceover as a screen-reader and seemed to work, but I have limited skillset there.
|
Hey @bennypowers upon reviewing the changes I have found a lot of a11y issues. This component needs to be refactored with some a11y remediation. I have a ticket to do that and want to work on it. Should I make the changes in this PR? How should we handle a possibly major refactor for this element? |
|
@bennypowers currently the component is only functional and useful to sighted mouse users without the remediation. |
|
Yeah this PR is the place for it. If the changes are going to be substantive let's try to plan them out a little first, so we can minimize back-and-forth in review |
97fa713 to
96206e7
Compare
kelsS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds aria-labelledby to jump links internal list
Related issues
What has changed and why
Testing instructions
NOTE TO testers: in my testing using VO and FF, the header text was repeated
list STANDARD SECTION standard section 5 items. Edgium and safari did not repeat, so i suppose that's a UA thing.Browser requirements
Your component should work in all of the following environments:
Ready-for-merge Checklist
Merging
Please squash when merging and ensure your commit message uses conventional commit formatting.
Be sure to share your updates with the [email protected] mailing list!