-
Notifications
You must be signed in to change notification settings - Fork 31
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
Warning added for headings with role="text" attribute issue #107 - a11y feature #111
Warning added for headings with role="text" attribute issue #107 - a11y feature #111
Conversation
…into jackdomleo7-master
I'm not sure what the linting error is due to, when I do npm run lint it doesn't show anything. Any help would be appreciated. |
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.
Some really good stuff here Emma! Just two very very minor changes needed then it's all good to go.
codes.md
Outdated
The highlighted element is a focusable element that is nested within another element with `aria-hidden="true"`. This means the focusable element is inaccessible to assistive technologies. Either remove the `aria-hidden="true"`, or restructure the HTML so that the focusable element is not nested within the element with `aria-hidden="true"`. [Read more about this here](https://web.dev/aria-hidden-focus). | ||
|
||
### W0010 |
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.
Just appears to be missing a -
at the beginning.
Also, to stick with consistency, could we use double quotes? role="text"
and wrap it in backticks so it is represented as code?
However, this is a good clear description!
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.
Changes made :)
@@ -29,4 +29,13 @@ describe("<headings>", () => { | |||
.should('eq', "ERROR (E0005): Headings should not skip levels.") | |||
}); | |||
}); | |||
|
|||
it('should not have role=text', () => { |
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.
Thank you for adding this! I've been meaning to get around to refactoring these tests. (No action needed)
Thanks for this PR! Yeah you can ignore the linting check (I'll override this when merging) - there's an open issue (#87) that I'm struggling to resolve around this. Also, don't worry about testing Safari, I'm confident this will work. |
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.
Nice one, thank you!
Types of changes
Description
This feature adds a warning to any headings h1 - h6 with the role="text" attribute that this will cause a loss of semantic meaning which could impact screen reader users. Resolves: #107
Link(s)
Screenshot(s)
Checklist:
x
if you have considered this but thought there was nothing to add or modify).contributors
section inpackage.json
(still put anx
if you have considered this but decided not to add yourself).test/index.html
to thetest/index.html
in themaster
branch).Help
I could not test in Safari because I do not have access to an Apple device.