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

Warning added for headings with role="text" attribute issue #107 - a11y feature #111

Merged
merged 9 commits into from
Oct 3, 2021

Conversation

EmmaDawsonDev
Copy link
Contributor

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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)

Screenshot 2021-10-03 135520

Checklist:

  • I have thoroughly read the CONTRIBUTING guidelines.
  • I understand my pull request will be thoroughly reviewed at high detail.
  • I understand the work in my pull request will only be available in the next version release of Checka11y.css and not in the current version release.
  • I confirm the work in this pull request is valid according to my findings and is not something for anything personal.
  • I have updated the README and/or features.md where and if applicable (still put an x if you have considered this but thought there was nothing to add or modify).
  • I have added myself to the contributors section in package.json (still put an x if you have considered this but decided not to add yourself).
  • I have checked I have not committed any accidental files.
  • I have tested all the main modern browsers (I.e. Chrome, Firefox, Edge, Safari - please leave this unchecked if there were any browsers listed you could not test and list them in the help section with details why you couldn't test that browser)
  • I have run the automated tests and added new ones to cover new code.
  • All new and existing a11y checks still work correctly (compare your local test/index.html to the test/index.html in the master branch).

Help

I could not test in Safari because I do not have access to an Apple device.

@EmmaDawsonDev
Copy link
Contributor Author

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.

@jackdomleo7 jackdomleo7 self-requested a review October 3, 2021 12:58
@jackdomleo7 jackdomleo7 added the a11y feature New feature or request for an a11y check label Oct 3, 2021
jackdomleo7
jackdomleo7 previously approved these changes Oct 3, 2021
Copy link
Owner

@jackdomleo7 jackdomleo7 left a 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
Copy link
Owner

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!

Copy link
Contributor Author

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', () => {
Copy link
Owner

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)

@jackdomleo7
Copy link
Owner

jackdomleo7 commented Oct 3, 2021

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.

@jackdomleo7 jackdomleo7 added the Hacktoberfest Hacktoberfest eligible label Oct 3, 2021
Copy link
Owner

@jackdomleo7 jackdomleo7 left a 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!

@jackdomleo7 jackdomleo7 merged commit 2905e43 into jackdomleo7:master Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y feature New feature or request for an a11y check Hacktoberfest Hacktoberfest eligible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning when using role="text" on heading elements
2 participants