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

Add aria-expanded attribute for accessibility #47

Merged
merged 3 commits into from
Oct 9, 2021

Conversation

jgrenat
Copy link
Contributor

@jgrenat jgrenat commented Sep 27, 2021

Hello !

As mentionned in #46, this PR adds the aria-expanded attribute :)

@luukdv
Copy link
Collaborator

luukdv commented Sep 30, 2021

Thanks!

The tests are a bit repetitive, can you change them to the following?

it(`has an expansion indicator (for accessibility) when toggled`, () => {
  render(<Hamburger toggled />)

  expect(screen.getByTestId('tilt')).toHaveAttribute('aria-expanded', 'true')
})

it(`doesn't have an expansion indicator (for accessibility) when closed`, () => {
  render(<Hamburger />)

  expect(screen.getByTestId('tilt')).toHaveAttribute('aria-expanded', 'false')
})

I'll define these attributes one level up and spread them into the individual components when I have time, so the repetition would be solved there as well.

@jgrenat
Copy link
Contributor Author

jgrenat commented Oct 4, 2021

Hello! Sure, I've updated the tests labels, let me know if you want me to change anything else 😀

@luukdv
Copy link
Collaborator

luukdv commented Oct 4, 2021

They're still a bit repetitive, can you change them (also the test contents) to the snippet above? So they'd be just 2 tests. Thanks in advance :)

@jgrenat
Copy link
Contributor Author

jgrenat commented Oct 4, 2021

Oh sorry, I missed that part, that's done now 😅

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.

2 participants