-
Notifications
You must be signed in to change notification settings - Fork 351
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 visible label and ARIA label to Dropdown widget #1845
base: main
Are you sure you want to change the base?
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ec98fa8) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1845 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1845 |
Size Change: +394 B (+0.03%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
> | ||
this article. If left blank, the value will | ||
default to "Select an answer". | ||
</a> |
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.
NIT: Perhaps linking the article this way:
Please see this
<a
href="https://www.w3.org/WAI/tips/designing/#ensure-that-form-elements-include-clearly-associated-labels"
target="_blank"
>
article on writing accessible labels
</a>
for more information. If left blank, the value will default to "Select an answer".
</SingleSelect> | ||
</div> | ||
<UniqueIDProvider scope="dropdown-widget" mockOnFirstRender={true}> | ||
{(ids) => ( |
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.
While we still have code that hasn't yet been converted, we are moving towards using React's useId
hook. Example usage.
this.props.ariaLabel || | ||
this.context.strings.selectAnAnswer | ||
} | ||
aria-labelledby={ids.get("dropdown-label")} |
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 wonder if this should be moved into the aria-label
logic. In the case where both a visible label and an aria label are specified, the visible label would be read instead of the aria label (since aria-labelledby
takes precedence). Perhaps aria-label
could be changed to:
aria-label={
this.props.ariaLabel ||
this.props.visibleLabel ||
this.context.strings.selectAnAnswer
}
/> | ||
<InfoTip> | ||
<p>Optional visible label</p> | ||
</InfoTip> |
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.
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.
Thanks so much for getting this accessible! Just a few comments.
Summary:
This commit adds two new fields to the Dropdown widget:
ariaLabel
andvisibleLabel
.Issue: LIT-1424
Test plan:
Screen reader walkthrough with
visibleLabel
:Dropdown.visible.label.mov
Screen reader walkthrough with
ariaLabel
only:(This will probably be most instances as the dropdown widget is mostly used inline, and the visible label makes things too cramped.)
Dropdown.ARIA.label.mov
Updates to widget editor:
You can also see examples in the course editor on this ZND: https://prod-znd-241122-danielle-dw2.khanacademy.org/devadmin/content/articles/dropdown-with-labels/x56e2ea23d30e405c