-
Notifications
You must be signed in to change notification settings - Fork 524
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 group component #1705
Feature/accessibility group component #1705
Conversation
packages/victory-core/src/victory-accessibility-group/victory-accessibility-group.js
Outdated
Show resolved
Hide resolved
|
||
return desc ? ( | ||
<g aria-label={this.props["aria-label"]} className={className} tabIndex={tabIndex}> | ||
<desc aria-describedby={this.props["aria-describedby"]} id={descId}> |
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.
I think aria-describedby
belongs on the g
tag. When there is a desc
, aria-describedby
should use the id of the desc
tag. When there isn't a desc
, it should use whatever user provided id string.
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.
ok cool, yeah I didn't get a lot out of looking up aria-describedby best practice, but in looking up reading on the <desc>
tag i see its placement on <desc>
is redundant. Will fix
|
||
return desc ? ( | ||
<g aria-label={this.props["aria-label"]} className={className} tabIndex={tabIndex}> | ||
<desc aria-describedby={this.props["aria-describedby"]} id={descId}> |
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.
Also, when there is a desc
, there needs to be a back up for if the user doesn't provide a descId
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.
OK found a better resource to understand the described by attribute, https://developer.paciellogroup.com/blog/2018/09/describing-aria-describedby/
as you had said, aria-describedby
and descId
should be identical, with aria-describedby
using the descId
I am wondering if aria-describedby
should even be a prop the user controls:
if(descId) => descId is required, aria-describedby will use the same input
else arial-label should suffice
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.
Hmm... I think aria-describedby
should still be a prop in the case where a user wants to describe the group with some other, non-victory element. If they provide both a desc
prop and a aria-describedby
prop, though, the user's aria-describedby
should be used to set the descId
so that they definitely match. If they provide only a desc
and no aria-describedby
we will need to generate an id to use for both aria-describedby
and descId
. So, I think descId
probably shouldn't be a prop.
packages/victory-core/src/victory-accessibility-group/victory-accessibility-group.js
Outdated
Show resolved
Hide resolved
@ljones87 This looks great! Good to merge, I think. |
b6c43ec
to
1d288f6
Compare
AccessibilityGroupComponent
tovictory-core
.groupComponent
prop to addaria-labels
anddesc
tags witharia-describedby
.aria-label
togroupComponent
optionVictoryClipContainer