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

Spinner item does not take other props #5317

Closed
bvandercar-vt opened this issue May 24, 2022 · 6 comments · Fixed by #5321
Closed

Spinner item does not take other props #5317

bvandercar-vt opened this issue May 24, 2022 · 6 comments · Fixed by #5321

Comments

@bvandercar-vt
Copy link
Contributor

Environment

  • Package version(s): 4.3.2
  • Operating System: Windows 10
  • Browser name and version: Chrome / All

Steps to reproduce

Trying to pass other props to the Spinner item, and they don't take effect. Specifically, trying to add aria-label to the Spinner item, and that prop is not being implemented.

Expected behavior

Ability to pass various other props to the Spinner item

@bvandercar-vt
Copy link
Contributor Author

This is essential for accessibility given the aria-progressbar-name rule: https://accessibilityinsights.io/info-examples/web/aria-progressbar-name/ .

So since Spinner automatically has role="progressbar", and "progressbar" needs aria-label or aria-labelledby, and you can't pass either of these to Spinner, there's no way to get it to pass a11y rules.

@adidahiya
Copy link
Contributor

  1. I'm all for a11y markup; we should add the required markup attributes to Spinner by default
  2. I would also support spreading arbitrary HTML attrs to Spinner, open to a PR for that

@bvandercar-vt
Copy link
Contributor Author

@adidahiya would you like me to contribute the PR?

Also regarding #1, what do you mean by this? I'm not sure we could have a default aria-label for Spinner, as a dev would always want to set their own unique value, and would sometimes want to use aria-labelledby instead of aria-label

@adidahiya
Copy link
Contributor

Ah ok, right, you'd want to set a custom aria-labelledby or aria-label. I was more so suggesting that we should add the suggested a11y markup attrs out of the box, if there are any we are missing which we can set statically. If there are no more we can do out of the box, that makes sense.

Yes I would be happy to review a PR from you which adds support for arbitrary attributes on the Spinner element. It might be tricky because we support the custom tagName prop, so those attributes could be of type HTMLAttributes<any> | SVGAttributes<any>... but it should be doable.

@bvandercar-vt
Copy link
Contributor Author

@adidahiya see my PR-- I didn't change the types, I just did it like how other elements that have both tagName and htmlProps do it

@adidahiya
Copy link
Contributor

Ah, that's right, I forgot that aria- props are built-in to JSX for React. That makes it easier to add this feature.

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

Successfully merging a pull request may close this issue.

2 participants