Accessibility enhancements#262
Conversation
| className={theme.collapse} | ||
| style={this.initialStyle} | ||
| aria-hidden={!isOpened}> | ||
| <div ref={this.onRefContent} className={theme.content} id={accessibilityId}> |
There was a problem hiding this comment.
Why id on content element? Why not set it in your children’s parent element? What its purpose in a11y context? I can see how area-hidden can be useful, but very much against adding any extra special props, especially when they can be avoided
There was a problem hiding this comment.
The value one passes to id needs to match the controlling element's aria-controls value (you can see this in action via the examples I created). We can leave it to the consumer to apply id as part of the child's top-level element, but I thought it'd be cool to encourage accessibility standards by exposing the optional prop (it's a bit more "in your face").
Lemme know if you're still against the idea of it as a prop. I can edit the examples and documentation accordingly. Personally, I'd advocate the other direction and require the accessibilityId prop, but it still won't guarantee anything... I just imagine people will want to understand the prop and adhere it to it.
There was a problem hiding this comment.
@nkbt curious on your thoughts. You may be adding a prop to your library, but the end result for the consumer is the same if a user follows best practices with accessibility. This way, you're simply encouraging best practices and helping to create a more accessible web.
There was a problem hiding this comment.
Look, I am very much against adding any props to the library unless it is absolutely impossible to do outside of the library and it is breaking intended functionality. I can understand the need for aria-hidden especially as it does not affect public API of the component itself at all, so it should be ok to add (though technically it still would be possible to overcome from outside, but the change is too unobtrusive). Everything else I don't see as part of the library, unfortunately, and would really like to avoid adding
Thank you for the change and all the reasoning though, I do appreciate your time and dedication in making web better. Legend 👍
There was a problem hiding this comment.
Sad to hear, but I've made adjustments that I think you'll be happy with!
nkbt
left a comment
nkbt
left a comment
There was a problem hiding this comment.
Thank you for the changes and great example. I will do some tests before merge, but looks good to me
|
Hello there, any plans on merging the PR? really need this 😬 |
|
I would ditch this library and use Reach UI's Accordion component: Accessibility is a requirement in that library. |
3 participants
As discussed in #255 , accessibility for collapses/accordions are difficult and not as simple as dealing with
aria-hidden. I've used it anyways, but also provided a prop and documentation on how to makereact-collapseusages more accessible.Resolves #255