-
Notifications
You must be signed in to change notification settings - Fork 593
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
Feature/accessibility #425
Conversation
2800c91
to
49f0efb
Compare
f6f40d4
to
8a13dde
Compare
@sarmeyer I was able to add some accessibility to nuka-carousel except a |
8a13dde
to
6441cc5
Compare
8b161d8
to
d973724
Compare
@@ -1150,7 +1150,16 @@ export default class Carousel extends React.Component { | |||
deltaX={tx} | |||
deltaY={ty} | |||
> | |||
{validChildren} | |||
{React.Children.map(validChildren, (child, index) => { | |||
const ariaProps = |
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.
@sarmeyer So I am using the same logic as before here but instead of using css {display: none}
I am using {aria-hidden: true}
. This way the images are still seen on the DOM keeping the look it had before but won't be read my the reader. Also, the tabbing order got messed up because I added tabindex: 1
to all the images. Now its just adding tabindex
the one the is visible.
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.
@sarmeyer @ElreyB So I just updated to this version and narrowed down a bug to this change that is highlighted here.
On mobile iPhone devices VoiceOver correctly focuses and reads the FIRST slide, but then skips all the rest of the visible slides. Instead it goes to the next div (i.e. some other button on the page) effectively causing a11y users to miss out on all the other slides.
Any ideas on what could be causing this? It seems to be aria-hidden
, because when I remove it, it works as before (used to be on 4.4.1).
@@ -477,6 +478,16 @@ export default class Carousel extends React.Component { | |||
case 40: | |||
this.previousSlide(); | |||
break; | |||
case 32: | |||
if (this.state.pauseOnHover && this.props.autoplay) { |
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.
@sarmeyer I tested this out but since you know a little more of the use cases for the carousel let me know if it actually works.
@@ -952,8 +952,9 @@ export default class Carousel extends React.Component { | |||
}; | |||
} | |||
|
|||
getStyleTagStyles() { | |||
return '.slider-slide > img {width: 100%; display: block;}'; | |||
getImgTagStyles() { |
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.
@sarmeyer For img
tags the width is set to 100% thus covering the outline
style for tabindex
. I added a focus style so that the tab outline can be seen. This style was hard to find so I changed the function name to make it easier.
Description
Adding accessibility to carousel
Fixes #197
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Manually test using ChromeVox and WAVE Evaluation Tool
Checklist: (Feel free to delete this section upon completion)
yarn lint
)yarn test
andyarn test-e2e
)