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

Accessibility and replacing title/description inside of the svg #639

Closed
bitboxer opened this issue Jan 21, 2020 · 9 comments
Closed

Accessibility and replacing title/description inside of the svg #639

bitboxer opened this issue Jan 21, 2020 · 9 comments

Comments

@bitboxer
Copy link

bitboxer commented Jan 21, 2020

It would be great if it would be possible to update the title and desc attribute of the svg to have a proper description based on the context where the svg is used. Also adding role="img" might be helpful, too.

I am currently adding all of that using the hooks ReactSVG provides, but having them inside of the library would be super nice.

@bitboxer
Copy link
Author

If you are okay with this proposal I would love to create the PR for this.

@tanem
Copy link
Owner

tanem commented Jan 21, 2020

Hi @bitboxer 👋

Sounds like you're using the API as intended here. Assuming I've understood your use-case correctly:

  • SVG desc and title elements can be added to the SVG via the beforeInjection callback.
  • role and aria-label elements can be automatically added to the outermost wrapping element via props.

I've created a live example here. Is this similar to what you've done?

I can understand why you're wanting a bit more convenience from this libraries API. A deliberate tradeoff was made some time ago which gave more power but pushed complexity such as this into user-land in order to prevent a larger API surface area. See here for context.

For that reason, I'm hesitant to expand the API. Still interested in seeing your proposal if you want to share it here though.

Finally, if this is too inconvenient for you, I'd suggest taking a look at react-inlinesvg which is a similar library but has a slightly different API which might suit your use-case better.

@bitboxer
Copy link
Author

Yes, the example is what I did in our codebase. My proposal would be adding a title and desc attribute and always adding a role="img" to the svg. If a title or desc is given, those will replace the content in the svg. I can totally understand the reasoning by not adding this, but I would suggest to add them anyway. This will make it easier to add accessibility features and maybe nudge people to add them. This will lead to a better experience for lots of people out there who are reliant on screenreader technology. Exposing developers to this will be worth it just for this little extra nudge 😉 .

@tanem
Copy link
Owner

tanem commented Jan 22, 2020

Yep, agree on making things more obvious in some way regarding a11y.

Still hesitant to expand the API in that way just yet though, so in the meantime, I'll add some documentation and an example based on what we've talked about here.

@tanem
Copy link
Owner

tanem commented Jan 26, 2020

Just merged #648. As per the PR desc., the FAQ section might not be in the best place, but we can update that in future, along with adding further FAQ based on past issue questions.

@tanem tanem closed this as completed Jan 26, 2020
@bitboxer
Copy link
Author

I understand your decision, but I really hope that you will change your mind in the near future and see that accessibility should not be hidden in a FAQ, but should be real attributes.

@tanem tanem removed their assignment Jan 29, 2020
@tanem
Copy link
Owner

tanem commented Jan 29, 2020

To clarify, the intent behind adding both an FAQ and live example was not to hide that information, but to make it more obvious, since previously it wasn’t documented at all.

It’s a stepping stone towards potentially modifying the API, so if anything changes in this regard in future, I’ll be sure to ping you 🙂

Repository owner locked as resolved and limited conversation to collaborators Jan 29, 2020
@tanem tanem mentioned this issue Feb 2, 2023
Repository owner unlocked this conversation Feb 2, 2023
@tanem
Copy link
Owner

tanem commented Feb 2, 2023

Re-read that last response of mine and it looked a little snarky, sorry about that @bitboxer 🤦‍♂️

Posting back to let you know that your suggestions are included in the linked PR (#2154). Feedback welcome. I'll update further once it's ready for merge.

@tanem
Copy link
Owner

tanem commented Feb 4, 2023

Released in v16.1.0 :shipit:

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

No branches or pull requests

2 participants