Skip to content

Improve a11y by using button with role switch#26

Merged
zombieJ merged 2 commits into
react-component:masterfrom
saitonakamura:a11y
Sep 12, 2018
Merged

Improve a11y by using button with role switch#26
zombieJ merged 2 commits into
react-component:masterfrom
saitonakamura:a11y

Conversation

@saitonakamura

@saitonakamura saitonakamura commented Sep 10, 2018

Copy link
Copy Markdown
Contributor

As for mozilla switch guidelines
Plus I switched span for button to get support for Enter and Space keys out of the box (now the browser will take care about it and that's why I deleted the test, cause enzyme doesn't fire onClick on such keypresses, but the browsers will do)

Fixes #22

As for [mozilla switch guidelines](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_switch_role)
Plus I switched `span` for `button` to get support for `Enter` and `Space`  keys out of the box (now the browser will take care about it and that's why I deleted the test, cause enzyme doesn't fire `onClick` on such keypresses, but the browsers will do)
@coveralls

coveralls commented Sep 10, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-2.9%) to 91.667% when pulling cbe38fb on saitonakamura:a11y into efa4993 on react-component:master.

Comment thread src/Switch.jsx
@saitonakamura

Copy link
Copy Markdown
Contributor Author

@zombieJ , are you busy or you're waiting for me to fix the coverage? Cause right now I don't why the coverage is decreased. I've deleted a test, but also deleted the code the test was testing. coveralls report doesn't help either

@zombieJ

zombieJ commented Sep 11, 2018

Copy link
Copy Markdown
Member

I'm OK with this.
@afc163 how do you think about this?

Comment thread tests/index.spec.js
expect(switcher.state().checked).toBe(true);
switcher.simulate('keydown', { keyCode: 13 });
expect(switcher.state().checked).toBe(false);
});

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.

Why remove this test case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

enzyme doesn't fire onClick on such keypresses, but the browsers will call onClick because it's button
Plus, if we'll try to test such behavior we will be actually testing a11y specification, which i think we shouldn't do

@afc163

afc163 commented Sep 11, 2018

Copy link
Copy Markdown
Member

cool

@afc163 afc163 closed this Sep 11, 2018
@afc163 afc163 reopened this Sep 11, 2018
@zombieJ zombieJ merged commit 43b67e6 into react-component:master Sep 12, 2018
@afc163

afc163 commented May 17, 2020

Copy link
Copy Markdown
Member

#45

@saitonakamura

Copy link
Copy Markdown
Contributor Author

@afc163 answered in the issue

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.

Consider improving accessibility support with aria-role

5 participants