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

Add aria-label for accessibility #645

Closed
wants to merge 5 commits into from
Closed

Conversation

judygab
Copy link

@judygab judygab commented Jun 7, 2019

When I ran accessibility test on AutoSuggest I received issues that the combobox and input fields don't have descriptions. To fix these issues I added aria-labels which are optional and the users can pass the values of it through props. This would also fix (#485 ), (#283 ), (#642 ). I'm also creating a pull request for react-autowhatever to pass down the aria-label to the suggestions container.

@rajeshwar1993
Copy link

Hi! I too am stuck with the same aria issues. It would be great if these could be available in master and updated to npm for installation as a package update.

Thanks!!

@xutopia
Copy link

xutopia commented Jan 10, 2020

Hello, my team is experiencing the same issue with our accessibility audit.
@moroshko, could you please help approve/merge this PR?

Thanks!
Tony

@zain1629
Copy link

I am having the same issue. Please approve the merge.

Thanks

@craigjdudley
Copy link

craigjdudley commented Feb 14, 2020

Also having this accessibility issue and waiting on the fix.

@TheRusskiy @moroshko

Thank you

@lranches-sfdo
Copy link

@judygab Any status on getting this PR merged? Would be greatly helpful to get this in.

@lranches-sfdo
Copy link

@moroshko ^^^^^^^^ 👍

@bfeister
Copy link

@judygab @moroshko +1 on this, our team is in need of some of these improvements, anything blocking the merge? We can help out if there is additional work needed.

@aberezkin
Copy link
Collaborator

@bfeister

blocking the merge?

Yes. It has conflicts.

@judygab
Copy link
Author

judygab commented May 11, 2020

@aberezkin it didn't show me conflicts on the pull request, anyways I followed the link from here, resolved them and committed it.
Screen Shot 2020-05-11 at 1 00 50 PM

@aberezkin
Copy link
Collaborator

aberezkin commented May 11, 2020

It looks like that you can achieve the same functionality using inputProps or/and custom renderSuggestionsContainer.

@judygab
Copy link
Author

judygab commented May 11, 2020

@lranches-sfdo @bfeister in my team, we just patched this module with my changes and it has been working fine since then. That is another workaround if this PR would not be accepted.

@lranches-sfdo
Copy link

@judygab That's helpful! We might have to go that route as well. Ideally, it would be good for everyone if it gets merged as this PR has been sitting for awhile it seems.

@aberezkin
Copy link
Collaborator

@judygab
What do you think about that?

#645 (comment)

@judygab
Copy link
Author

judygab commented Jun 12, 2020

@judygab
What do you think about that?

#645 (comment)

thanks, inputProps didn't give me the same result, but haven't tried renderSuggestionsContainer.

@mlp73
Copy link

mlp73 commented Aug 19, 2020

Same issue here +1
Looks like the PR does not have any blockers any more. Ready for merge?
@moroshko

@hugoholgersson
Copy link
Collaborator

I agree with @aberezkin that it would be cleaner to send aria-label via inputProps. Why didn't that work for you @judygab?

@pimterry
Copy link
Collaborator

I totally agree with the goal here, but I think the implementation isn't quite right (in part because this has been sat here for a while, sorry about that!). I'd love to merge this PR though. I think there's a few things required:

  • It looks like this is expecting to build on top of Add aria-labels react-autowhatever#48 in autowhatever. That's no longer going to work, as that code has all been inlined into this repo directly: https://github.com/moroshko/react-autosuggest/blob/master/src/Autowhatever.js
  • It looks like this PR (in combination with that other PR) adds a new ariaLabel prop, and then set an input aria-label (that's just fixed as 'suggestions', I think?) and a container aria-label (based on the ariaLabel prop). I think the slightly better approach would be:
    • Don't add a new ariaLabel prop
    • Use the existing inputProps prop to configure the input aria label. We should include a default value for this in our default props, and then users who want to override it can easily do so using inputProps themselves.
    • Add a new containerProps prop to configure the container aria label, by similarly setting an overridable default for that. Autowhatever already exposes a containerProps prop, so it's just a matter of exposing this from Autosuggest too (this is related to Add containerProps property for arbitrary props on the container #592, although that's quite out of date now, and no longer mergeable). This is also useful for anybody who wants to configure the container props in other ways, for ARIA or anything else too.

Does that make sense? If anybody wants to take that on, I'd love to get this finished up & merged!

@pimterry
Copy link
Collaborator

Hi @judygab are you still interested in this? I'd be happy to merge it if you've got a minute to look into the comments above. If not no worries, but I'll close this PR in a few days.

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.