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

Accept additional props for labeling icons #907

Closed
2 tasks
joshblack opened this issue Jan 27, 2023 · 6 comments · Fixed by #932
Closed
2 tasks

Accept additional props for labeling icons #907

joshblack opened this issue Jan 27, 2023 · 6 comments · Fixed by #932
Assignees
Labels

Comments

@joshblack
Copy link
Member

We should make sure the React icon components accept additional props for labeling icons, including:

  • aria-labelledby
  • Support for adding a <title> tag

For the title tag, I think we have two options:

  • A title prop that accepts either a string or React.ReactNode (in case the user wants to provide an id to the title
  • Accept children as a prop and users can provide a title as children to the icon component
@mattcosta7
Copy link

mattcosta7 commented Jan 27, 2023

@joshblack I think we should also accept all of React.SVGAttributes<SVGElement> as valid IconProps as well, to allow passing additional things like aria-labelledby if necessary, or even just data- attributes and the like

While we're here, should we also make these ref forwarding? that might bump the peer from >=15 to something higher though. I don't think it's an unreasonable time to do so?

@joshblack
Copy link
Member Author

@mattcosta7 I'd totally be down for the forwardRef change 🔥

I made #910 to track this. Do you think it'd be something we should try and feature detect to ship it in a minor release or do you think it's worth trying to do a major release for the icons package and increase the deps to >=16.3?

@mattcosta7
Copy link

@mattcosta7 I'd totally be down for the forwardRef change 🔥

I made #910 to track this. Do you think it'd be something we should try and feature detect to ship it in a minor release or do you think it's worth trying to do a major release for the icons package and increase the deps to >=16.3?

I haven't tried to feature detect it, but no objections if there's a good path there!

@pveierland
Copy link

Came here to file the same issue. It also seems sensible to set the value of aria-label to the title value if one is provided.

@broccolinisoup
Copy link
Member

👋🏻 @green6erry Are you actively working on this ticket? I see a linked PR but that was back in March. I have been working on some octicon changes #943 and might have some capacity to pick this up if you are not planning to continue working on this? Totally cool if you are though! 🙌🏻 Just wanted to ask!

@lesliecdubs
Copy link
Member

@green6erry - ping to check out the draft PR and see if we can move it toward review. Thanks!

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

Successfully merging a pull request may close this issue.

6 participants