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

Pass something besided string to getOptionLabel()? #4134

Closed
jkjustjoshing opened this issue Jul 21, 2020 · 6 comments · Fixed by #4414
Closed

Pass something besided string to getOptionLabel()? #4134

jkjustjoshing opened this issue Jul 21, 2020 · 6 comments · Fixed by #4414
Assignees
Labels
category/accessibility Issues or PRs related to accessibility category/documentation Issues or PRs about documentation or the website itself

Comments

@jkjustjoshing
Copy link
Contributor

The docs for getOptionLabel indicate that the only valid return type is a string.
image

However, it seems like a return value of any React node will work, such as the following:

getOptionLabel={(data) => <b>{data.name}</b>}

Should/can the docs be updated to show that anything React can render is an acceptable return type?

@jkjustjoshing
Copy link
Contributor Author

Also, if this is accepted, the DefinitelyTyped type should be updated

@jkjustjoshing
Copy link
Contributor Author

I just realized that this breaks the aria-live region / screen reader support, since the return value for getOptionLabel is concatenated with a bunch of other strings. When that's a JSX object you get the screen reader reading [object Object] which obviously isn't good. Closing ticket.

@jkjustjoshing
Copy link
Contributor Author

Actually, would it make sense to log a console warning if a non-string is passed? Most people probably aren't verifying that their screen readers still work, and might do what I did: try it out with some JSX, realize it works, and leave it there.

@jkjustjoshing jkjustjoshing reopened this Jul 24, 2020
@bladey bladey added category/documentation Issues or PRs about documentation or the website itself issue/enhancement issue/reviewed Issue has recently been reviewed (mid-2020) labels Aug 24, 2020
@ebonow
Copy link
Collaborator

ebonow commented Dec 13, 2020

@jkjustjoshing Thanks for bringing this up.

It's likely intended that the types in the documentation should be updated to return a Node. I will add a note to follow through on this.

Regarding aria-live regions, I also agree this can break screen-readers. I'd like to propose a solution and see if this sounds good to you.

  1. If JSX is passed in, parent node should contain an aria-label.
  2. If JSX is passed in, aria-live config should use this value.

Thoughts?

@ebonow ebonow self-assigned this Dec 13, 2020
@ebonow ebonow added category/accessibility Issues or PRs related to accessibility awaiting-author-response Issues or PRs waiting for more information from the author and removed issue/reviewed Issue has recently been reviewed (mid-2020) labels Dec 13, 2020
@ebonow
Copy link
Collaborator

ebonow commented Jan 27, 2021

I am currently looking at accessibility within react-select and these functions are critical to it working as intended. I have linked to this issue in #4407 with the hopes of getting more attention around the desired implementation and enforcement of best practices.

@ebonow
Copy link
Collaborator

ebonow commented Mar 1, 2021

I have updated the documentation for this prop to include concerns around accessibility in #4414 which should offer more transparency about a11y implications.

@ebonow ebonow removed the awaiting-author-response Issues or PRs waiting for more information from the author label Mar 1, 2021
@ebonow ebonow linked a pull request Mar 1, 2021 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/accessibility Issues or PRs related to accessibility category/documentation Issues or PRs about documentation or the website itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants