Accept additional props for labeling icons

primer / octicons

A scalable set of icons handcrafted with <3 by GitHub

https://primer.style/foundations/icons

MIT License

8.27k stars 827 forks source link

Accept additional props for labeling icons #907

Closed joshblack closed 1 year ago

joshblack commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

I made https://github.com/primer/octicons/issues/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 commented 1 year ago

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

I made https://github.com/primer/octicons/issues/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 commented 1 year ago

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 commented 1 year ago

👋🏻 @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 https://github.com/primer/octicons/pull/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 commented 1 year ago

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