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

Replace title tag in SVG with aria-labelledby #2777

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jasinskidev
Copy link
Contributor

The only thing that this PR does is to replace title tag in SVGs with aria-labelledby.
Why? We had complaints from SEO team about this and their recommendation was to use one unified title tag or delete it.
I investigated that and official documentation says that title tag is not recommended and we should use aria-labelledby so I replaced it in every SVG with title tag.

<title>
add more
</title>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512" aria-labbeledby="add more">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be aria-labelledby (btw. there's a typo here) but aria-label. aria-labelledby needs another item with the id provided between "" to be able to label this element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That what's happening when you blindly copy paste from the docs without thinking 😄 Yes I know, I will change it, thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but to be honest I'm not sure if the svg does actually need any label - it just has a presentational role. I'd suggest to give it alt="" as this suggests to the screen readers that it doesn't matter for the page content

@matzimowski
Copy link
Contributor

matzimowski commented Sep 15, 2023

i'd would advise to remove title and do not replace it with another attribute, since we already remove it on a build step: https://github.com/brainly/style-guide/pull/2578/files

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.

None yet

3 participants