-
Notifications
You must be signed in to change notification settings - Fork 333
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
feat(dom): Add labels to buttons #1234
Conversation
That's weird, there's already textContent in both of those buttons:
I wonder if this is happening when the query is
|
This is happening when we use detachedMediaQuery="" (blank) , If you write something then it replace button with form. I also passed translations but it not worked. |
If only the |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3665cff:
|
Hi there, thanks for your contribution. I looked into it and it seems there are 2 related issues we could fix:
const detachedSearchButton = createDomElement('button', {
// ...
title: translations.detachedSearchButtonTitle,
});
const detachedSearchButton = createDomElement('button', {
// ...
id: labelProps.id,
}); With these changes, screen reader should correctly read the button and link it to the Autocomplete role. |
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.
Just some final touches and it will be good to go for me. In addition to the suggestions, you'll need to set the default value for translations.detachedSearchButtonTitle
, in getDefaultOptions.ts:
const defaultTranslations: AutocompleteTranslations = {
clearButtonTitle: 'Clear',
detachedCancelButtonText: 'Cancel',
+ detachedSearchButtonTitle: 'Search',
submitButtonTitle: 'Submit',
};
packages/autocomplete-shared/src/js/AutocompleteTranslations.ts
Outdated
Show resolved
Hide resolved
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 again for your contribution, this passes tests locally.
Summary
fixes #1183
Result