Skip to content
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

fix(standalone): use aria label from input #276

Merged
merged 1 commit into from
Jan 16, 2019
Merged

fix(standalone): use aria label from input #276

merged 1 commit into from
Jan 16, 2019

Conversation

kontrollanten
Copy link
Contributor

Summary

Use aria-label property from given input element on the new input element.

Result

When using standalone version (with places.js for instance) the aria-label on the target input element will be forwarded to the input element created by autocomplete.js.

@coveralls
Copy link

coveralls commented Dec 17, 2018

Coverage Status

Coverage remained the same at 88.947% when pulling 99b905e on kontrollanten:fix-aria_label into 84b1714 on algolia:master.

@Haroenv
Copy link
Contributor

Haroenv commented Dec 18, 2018

This looks correct, but I will not have time to review this request until after the holidays. Could you describe which behaviour you are trying to fix? I thought there were already labels?

@kontrollanten
Copy link
Contributor Author

I'm using places.js and I have this element

<input aria-label="This is my label" id="autocomplete" />

When I call places({ container: document.getElementById('autocomplete') }) the aria-label disappears. What I could see this problem was only in the standalone version. In the autocomplete version it'll be dressed with aria-label here https://github.com/algolia/autocomplete.js/blob/master/src/autocomplete/typeahead.js#L581

@kontrollanten
Copy link
Contributor Author

@Haroenv A friendly reminder.

Copy link

@JonathanMontane JonathanMontane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and good catch!

Autocomplete.js has an ariaLabel option exposed (although it seems that it's not correctly forwarded, as you noticed). link

It would probably be better if your fix uses the options.ariaLabel and uses the input's aria-label attribute as a fallback.

Something like that:

  ...
  ariaLabel: options.ariaLabel || input.getAttribute('aria-label')

This would probably have a less surprising behaviour when calling autcomplete('#search-box', { ariaLabel: 'my-label', ... })

Copy link

@JonathanMontane JonathanMontane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Haroenv Haroenv merged commit 4b94466 into algolia:master Jan 16, 2019
@kontrollanten kontrollanten deleted the fix-aria_label branch January 16, 2019 09:28
@kontrollanten
Copy link
Contributor Author

Can we have a release for this soon? :)

@Haroenv
Copy link
Contributor

Haroenv commented Feb 21, 2019

I released 0.36.0 earlier today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants