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 role prop to icons #13

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Add role prop to icons #13

merged 5 commits into from
Mar 13, 2024

Conversation

palkerecsenyi
Copy link
Contributor

I've added a role prop to the Icon component that adds the role attribute to the SVGs that get rendered.

This is useful for accessibility, e.g. adding role="presentation" to mark an icon as decorative.

I think I've generated the icons correctly, but if not I'm happy to make any changes.

@finnbear
Copy link
Owner

finnbear commented Mar 4, 2024

Thanks, I like this idea. One issue I noticed is that some SVGs already have a role attribute. Here is one example:

< svg role = "img" viewBox = "0 0 24 24" xml
ns = "http://www.w3.org/2000/svg" data - license = "From https://github.com/simple-icons
/simple-icons - Licensed under CC0; check brand guidelines" width = { width . clone () }
height = { height . clone () } onclick = { onclick . clone () } oncontextmenu = { oncon
textmenu . clone () } class = { class . clone () } style = { style . clone () } role = {
role . clone () }
...

If you update your PR, and are able, please avoid checking in all the generated files. They stretch the limit of GitHub, making review harder. I'd rather just regenerate them on my end.

Also note that until #12 is fixed, I can merge this but not update crates.io.

@palkerecsenyi
Copy link
Contributor Author

@finnbear thanks for your reply, sorry for not getting back sooner! I've reverted my commits and re-added my changes without regenerating the files so hopefully it shouldn't crash your browser this time!

I've also added a regex (similar to height/width) to remove the existing role props from the icons. I think this is an acceptable way to handle it — the icon libraries shouldn't be assuming what the role of the icon will be IMO as that very much depends on how the implementing website uses it. I've tried it and the simple-icons are now generating without their default role prop for me.

Hope this works, let me know if you'd like any other changes :)

@finnbear
Copy link
Owner

finnbear commented Mar 13, 2024

Thanks, I appreciate this!

Will regenerate the icons in another commit. (Edit: Done)

As I mentioned, will need to fix #12 before I can publish a new version on crates.io. No ETA, but at least now there is a reason to do so.

@finnbear finnbear merged commit 9a2e734 into finnbear:main Mar 13, 2024
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.

2 participants