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

feat(gatsby-remark-images): add option to use alt text as caption fallback #14379

Merged

Conversation

chasemccoy
Copy link
Contributor

@chasemccoy chasemccoy commented May 28, 2019

gatsby-remark-images has a wonderful option to show captions under images using the title attribute, or the alt attribute if no title is provided. This causes us some issues, because we have a lot of images that we don't want captions for, but we do want to provide alt text (for accessibility reasons).

This PR adds a new configuration option to the gatsby-remark-images plugin:

fallbackToAltTextForCaptions - (defaults to true) If showCaptions is set to true and the title attribute is empty, this specifies whether the alt attribute should be used as a caption. If true it will be used, and if false the caption will be omitted if there is no title attribute.

This is my first time contributing to Gatsby, so my apologies if I get any of this wrong. I am not sure what we want to call the option (I went with fallbackToAltTextForCaptions because it was clear, but happy to pick a less verbose name). I also wrote a test to cover this case and updated the README of the plugin to document the new option.

@chasemccoy
Copy link
Contributor Author

@DSchau I think you helped review the PR to accept the alt attribute as a fallback caption (#13489), so tagging you here for visibility.

@chasemccoy chasemccoy force-pushed the topics/gatsby-remark-images-alt-captions branch from 149b43e to 99dcca4 Compare May 28, 2019 18:47
@m-allanson
Copy link
Contributor

m-allanson commented May 31, 2019

Thanks @chasemccoy! Thinking aloud, here's an alternative approach (maybe this is a terrible idea though?) - instead of creating a new option, we could override the existing showCaptions option.

  • Passing a boolean true or false would retain the existing behaviour
  • Passing an array with multiple values like ["alt", "title"] or ["title", "alt"] would specify the order that the fields should be checked. The default source value would be the first string in the array, with any further items acting as fallback source values
  • Passing a single value array would specify the only source value, e.g. ["title"]

This is similar to how the withWebP and tracedSVG options work, and hopefully covers any ordering someone would like to use.

If we really wanted to, we could allow passing a string title or alt as a shortcut for a single value array.

What are your thoughts @DSchau and @chasemccoy?

@chasemccoy
Copy link
Contributor Author

@m-allanson Hmm, I like that idea quite a lot! Question: would passing true be equivalent to ["title", "alt"]? I believe that is the default behavior now (use title first, alt second if the showCaption option is true)

@chasemccoy
Copy link
Contributor Author

chasemccoy commented May 31, 2019

@m-allanson Okay, I just pushed up my first stab at this behavior. I wrote some new tests as well to make sure everything works as expected, and I updated the README to reflect this new behavior. My implementation assumes that true is equivalent to ["title", "alt"], but let me know if that's not the case.

@chasemccoy chasemccoy force-pushed the topics/gatsby-remark-images-alt-captions branch from 51df8b8 to 72091fe Compare May 31, 2019 19:13
@chasemccoy chasemccoy force-pushed the topics/gatsby-remark-images-alt-captions branch from 72091fe to f016b4c Compare May 31, 2019 19:47
@freiksenet
Copy link
Contributor

There is a linter failure in the switch block, could you fix it please? Otherwise looking good. CC @m-allanson

@chasemccoy chasemccoy requested a review from a team as a code owner June 10, 2019 15:02
@chasemccoy
Copy link
Contributor Author

@freiksenet Okay, it should be fixed now! Thanks!

@chasemccoy
Copy link
Contributor Author

@m-allanson @freiksenet Could y'all give a 👍 on this if it's ready? Would love to get this published so that my team can use it 🙂

@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jun 13, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Looks great! I added another test and fixed a small that it doesn't fallback to alt when title is the only attribute specified & can't be found.

@wardpeet wardpeet merged commit e5171f1 into gatsbyjs:master Jun 13, 2019
@gatsbot
Copy link

gatsbot bot commented Jun 13, 2019

Holy buckets, @chasemccoy — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@m-allanson
Copy link
Contributor

Thanks @chasemccoy! 🎉

@chasemccoy
Copy link
Contributor Author

Thank you all so much!

@wardpeet
Copy link
Contributor

Published in [email protected]

mxxk pushed a commit to mxxk/gatsby that referenced this pull request Jun 21, 2019
…lback (gatsbyjs#14379)

Add an array of node fields to the showCaption option to give more flexibility over the image caption.
johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019
…lback (gatsbyjs#14379)

Add an array of node fields to the showCaption option to give more flexibility over the image caption.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants