Skip to content

fix(a11y) Fix aria-label being not present when the option is set#237

Closed
tryzniak wants to merge 4 commits into
algolia:masterfrom
tryzniak:fix/input-aria-label
Closed

fix(a11y) Fix aria-label being not present when the option is set#237
tryzniak wants to merge 4 commits into
algolia:masterfrom
tryzniak:fix/input-aria-label

Conversation

@tryzniak
Copy link
Copy Markdown

@tryzniak tryzniak commented Jun 27, 2018

Summary

I found a bug where an aria-label attribute could not be set using <input aria-label="blah" /> or autocomplete.js options. It just wasn't there.

Result

This PR fixes this issue by passing ariaLabel from autocomplete options to Typeahead.

@Haroenv
Copy link
Copy Markdown
Contributor

Haroenv commented Jun 27, 2018

Interesting that this didn't pass the test case made in #195. Do you think you can add a test case which failed before this PR? It should be in a file like this one: https://github.com/algolia/autocomplete.js/blob/master/test/unit/typeahead_spec.js

@tryzniak
Copy link
Copy Markdown
Author

I'll try to write a regression test. It's an integration issue between Typeahead and autocomplete modules though. So I think https://github.com/algolia/autocomplete.js/blob/master/test/unit/typeahead_spec.js won't work here, because it's a unit test. But thank you for the nudge :)

@tryzniak
Copy link
Copy Markdown
Author

Wow I've been trying to run integration tests for an hour but to no avail. Somehow I need to write a test that autocomplete's options.ariaLabel is passed to Typeahead. Any ideas?

@vvo
Copy link
Copy Markdown
Contributor

vvo commented Jun 27, 2018

Only tests PRS opened by contributors will have their tests passing because we use travis encrypted variables

@vvo
Copy link
Copy Markdown
Contributor

vvo commented Jun 27, 2018

(otherwise launch travis tests manually and it works for contributors I guess)

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 29, 2018

Coverage Status

Coverage remained the same at 88.854% when pulling 78f6942 on tryzniak:fix/input-aria-label into 0e65fee on algolia:master.

@tryzniak tryzniak force-pushed the fix/input-aria-label branch from ccb50da to 6afe041 Compare July 3, 2018 09:42
@tryzniak tryzniak force-pushed the fix/input-aria-label branch from 6afe041 to 129c31e Compare July 3, 2018 09:46
@tryzniak
Copy link
Copy Markdown
Author

tryzniak commented Jul 3, 2018

@Haroenv @vvo , I looked at travis' logs and they output Not running any tests. Which is suspicious. Maybe tests are not running at all? Because I even added a failing test just to see that they were not passing, but instead they were. Or am I doing something wrong here haha.

@kontrollanten
Copy link
Copy Markdown
Contributor

I created PR #276 before I saw this. In my solution the aria-label is read from the input element though. I think that solution feels more natural. What do you say, @tryzniak ?

@tryzniak
Copy link
Copy Markdown
Author

tryzniak commented Dec 18, 2018 via email

@francoischalifour
Copy link
Copy Markdown
Contributor

Autocomplete v0 has been in low maintenance mode for a few years and we just released Autocomplete v1 so I'm closing this.

Thanks for your contribution @tryzniak!

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.

7 participants