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

fix(gatsby-remark-images): enable creating img tag with empty alt attribute #27218

Merged
merged 8 commits into from
Oct 2, 2020
Merged

Conversation

motou
Copy link
Contributor

@motou motou commented Oct 1, 2020

Description

add a keyword GATSBY_EMPTY_ALT to enable that an img tag with an empty alt attribute can be created

Documentation

add the keyword description @gatsbyjs/documentation

Related Issues

Fixes #20179

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 1, 2020
@pieh pieh added topic: remark/mdx Related to Markdown, remark & MDX ecosystem and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 1, 2020
@pieh pieh self-assigned this Oct 1, 2020
@pieh
Copy link
Contributor

pieh commented Oct 1, 2020

The _SKIP_ seems pretty sensible as a keyword, but I wonder if it shouldn't be both more descriptive and less likely to accidentally hit the reverse situation (someone tries to actually have that as alt and then tries to figure out why instead of that alt it's empty string).

My proposal would be something like GATSBY_EMPTY_ALT - this feature is gatsby specific, and hopefully whoever hit that case would at least be able to search for it easier than just _SKIP_?

Other than that - do you think you could add some tests for this behaviour?

I realize we don't really have alt specific tests - we do have accidental testing of this due to snapshots, but it is accidental. I think something like structure below (don't need to add snapshots)

describe(`image alt attribute`, () => {
  it(`alt tag`, async () => {
    const imagePath = `images/my-image.jpeg`
    const content = `![testing-if-alt-is-correct](./${imagePath} "some title")`

    const nodes = await plugin(createPluginOptions(content, imagePath))
    expect(nodes.length).toBe(1)

    const node = nodes.pop()
    const $ = cheerio.load(node.value)
    expect($(`img`).attr(`alt`)).toEqual(`testing-if-alt-is-correct`)
  })
})

for few scenarios we already have:

  • alt tag is specified -> make sure that markup is using that
  • alt tag is not specified -> check if filename is used
    and one for using the keyword for "empty alt" -> check if it's actually empty

that can be added to https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-remark-images/src/__tests__/index.js will go long way in assuring we don't regress in future

@motou
Copy link
Contributor Author

motou commented Oct 2, 2020

Hi @pieh thanks for reviewing and providing feedbacks! I like the idea of the gatsby special tag and will adopt them in the PR. Tests will be not ignored ;)

@motou motou changed the title fix: enable creating img tag with empty alt attribute fix(gatsby-remark-images): enable creating img tag with empty alt attribute Oct 2, 2020
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

This is perfect, thank you!

@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Oct 2, 2020
@pieh pieh merged commit 9f0b545 into gatsbyjs:master Oct 2, 2020
@motou
Copy link
Contributor Author

motou commented Oct 2, 2020

Cool, thanks for merging. When is the new version 3.3.33 available? ;)

@tw15egan
Copy link

tw15egan commented Oct 2, 2020

Thanks, everyone for fixing this upstream so quickly 🙏 ❤️

@pieh
Copy link
Contributor

pieh commented Oct 6, 2020

Published in [email protected]

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…ribute (gatsbyjs#27218)

* fix: define a new constant for empty alt

* fix: generate empty alt attribute for EMPTY_ALT

Currently EMPTY_ALT is defined as _SKIP_

* chore: prettifier the codes

* chore: add documentation

* chore: add keyword description in README

* fix: incorprate review comments

* fix: add more unit tests

* fix: fix unit test error

Co-authored-by: Zhen Wang <[email protected]>
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 topic: remark/mdx Related to Markdown, remark & MDX ecosystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to include an image with empty alt attribute in Markdown
4 participants