-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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 basic accessibility fixes #2215
Add basic accessibility fixes #2215
Conversation
Deploy preview ready! Built with commit 2be53d0 |
@@ -26,7 +26,7 @@ class BlogPostPreviewItem extends React.Component { | |||
}} | |||
> | |||
<img | |||
alt={`Avatar for ${post.frontmatter.author.id}`} | |||
alt="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes the existing alt text and replaces it with a blank string - I don't think that was desired?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing content here is not really beneficial, as the exact same string (name of the author) is provided shortly after, so that content would be read out twice by screen readers. See this guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for being cautious :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for being informative! I didn't realise that unnecessary alt text could actually be damaging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the premise that image tags should have alt
attributes; but I don't think defining blank alt
attributes throughout the codebase adds anything over not having them.
If this was to be merged, the alt
attributes being defined should have values.
Thanks for taking the time to improve this, it's really appreciated. In the cases where the alt text is empty van we add something semantically relevant instead? I don't think empty text is generally better than no attribute |
@djm, @jquense – thanks for taking your time to review this so quickly. Have a look at the following article as for why providing an empty ALT text is different from not providing ALT attribute at all — Basically it means that a given image could be safely ignored by assistive technologies such as screen readers. If no ALT text is provided, screen readers would try to read out the filename from the image path which can be very confusing. WCAG 2.0 (W3C's accessibility guidelines) also recommends the same thing: https://www.w3.org/TR/WCAG20-TECHS/H67.html Oftentimes empty |
@theanxy ah, thanks, TIL! In that case, leaving some of them blank makes sense to me. But shouldn't pictures that are part of content like "panda-group-eating-bamboo.jpg" have relevant alt text? |
@djm Totally — good call! Fixed |
Great stuff @theanxy! Thanks for cleaning things up and teaching us some about the "alt" property. |
Hiya @theanxy! 👋 This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here. Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! 💪💜 |
@jlengstorf Thank you! I appreciate it and I'll see if I'll be able to help with anything else 💜 |
A couple of small fixes to improve the overall accessibility, both in code samples and on the landing page.
My main focus was fixing the tutorial so that all images would have ALT text. I find it important to spread good practices especially in the documentation.