Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

feat: 🎸 added jest-axe#2650

Merged
ovflowd merged 8 commits into
nodejs:mainfrom
benhalverson:jest-a11y
Aug 22, 2022
Merged

feat: 🎸 added jest-axe#2650
ovflowd merged 8 commits into
nodejs:mainfrom
benhalverson:jest-a11y

Conversation

@benhalverson

@benhalverson benhalverson commented Aug 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run lint:js -- --fix and/or npm run lint:md -- --fix for my JavaScript and/or Markdown changes.
    • This is important as most of the cases your code changes might not be correctly linted
  • I have run npm run test to check if all tests are passing, and/or npm run test -- -u to update snapshots if I created and/or updated React Components.
  • I have checked that the build works locally and that npm run build work fine.
  • I've covered new added functionality with unit tests if necessary.

Description

Adds the jest-axe dep so we can write tests to test for accessibility.

code example from their docs:

const React = require('react')
const App = require('./app')

const { render } = require('@testing-library/react')
const { axe, toHaveNoViolations } = require('jest-axe')
expect.extend(toHaveNoViolations)

it('should demonstrate this matcher`s usage with react testing library', async () => {
  const { container } = render(<App/>)
  const results = await axe(container)
  
  expect(results).toHaveNoViolations()
})

@benhalverson benhalverson marked this pull request as ready for review August 18, 2022 04:52
@codecov-commenter

codecov-commenter commented Aug 18, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2650 (0110c21) into main (65c6299) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2650   +/-   ##
=======================================
  Coverage   84.19%   84.19%           
=======================================
  Files         102      102           
  Lines        1120     1120           
  Branches      308      308           
=======================================
  Hits          943      943           
  Misses        171      171           
  Partials        6        6
Impacted Files Coverage Δ
src/components/Header/index.tsx 94.44% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ovflowd

ovflowd commented Aug 18, 2022

Copy link
Copy Markdown
Member

I loved this change! Big-yes for Accessibility!! 🎸

I would, if possible, ask, instead of using English strings, to create translation keys for your aria-labels and add them on the src/i18n/locales/*.json!

Also, can we add a section just for Accessibility within CONTRIBUTING.md?

@ovflowd

ovflowd commented Aug 19, 2022

Copy link
Copy Markdown
Member

I find the error "Interactive controls must not be nested (nested-interactive)" very intriguing. Is there a way to ignore this specific one?

As we have code inline to help with accessibility for the search input.

i.e.:

  • When tabbing to the search container it will actually focus on the input
  • If you start typing it will type on the search input as normally
  • The reason why the div is also interactive is so when clicking on the icon it also opens/expands the input.
  • A possible fix could be to change the role from button to presentation. WDYT?

ovflowd
ovflowd approved these changes Aug 19, 2022
View reviewed changes

@ovflowd ovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved, but I think we should add axe to all pages, no?

@mikeesto

mikeesto commented Aug 20, 2022

Copy link
Copy Markdown
Member
  • The reason why the div is also interactive is so when clicking on the icon it also opens/expands the input.

Clicking the icon does not expand the input for me - on Chrome. I have to click the label (Search).

@ovflowd

ovflowd commented Aug 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Clicking the icon does not expand the input for me - on Chrome. I have to click the label (Search).

Excuse me 😅 , I meant onKeyPress, not onClick. But it's still interactive because of its role field. Which needs to be removed/changed as I removed the onClick.

@ovflowd ovflowd mentioned this pull request Aug 21, 2022
Merged
5 tasks
@benhalverson

benhalverson commented Aug 21, 2022

Copy link
Copy Markdown
Member Author

Approved, but I think we should add axe to all pages, no?

Yes we should. They can be added in follow up PR's

@ovflowd ovflowd merged commit 371b952 into nodejs:main Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ovflowd ovflowd ovflowd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benhalverson @codecov-commenter @ovflowd @mikeesto