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-image): When art-directing images, default the padding aspect ratio to first image without a media query when no media query matches #21431

Merged
merged 2 commits into from
Mar 18, 2020

Conversation

mjmaurer
Copy link
Contributor

Description

When art directing multiple images and no media query matches, the current implementation chooses the first image with a media query.

This PR will choose the first image without media if no images match current window. This reflects what one would expect from the documentation (right now the mobile image would never be selected)
The documentation reflects what one would expect from mobile-first design.

I don't believe this is a breaking change. Any client that has media with every image will continue to work the same way. Any client that has an image without media probably expects the behavior that this PR implements.

Related Issues

Addresses documentation error mentioned above
Addresses #15189 when adding a "default" image (without media query)
Related to art direction support in #13395

@mjmaurer mjmaurer requested a review from a team as a code owner February 13, 2020 17:46
@wardpeet wardpeet self-assigned this Mar 4, 2020
@waverly
Copy link

waverly commented Mar 10, 2020

This would be really useful - is this going to merge anytime soon?

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

nice bug fix! Thanks for finding & fixing

@KyleAMathews KyleAMathews merged commit df7e93d into gatsbyjs:master Mar 18, 2020
@gatsbot
Copy link

gatsbot bot commented Mar 18, 2020

Holy buckets, @mjmaurer — 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!

@andrezimpel
Copy link

I am using gatsby-image 2.3.4 and unfortunately this does not work for me. When the page is loaded it is still using the aspect ratio of the first source instead of the matched one. I am logging out the image.aspectRatio and that is indeed correct when loading the page, although the paddingBottom does not get updated. As soon as I navigate to a different page and then come back it works.

Here is a preview link: https://preview.zuckerjagdwurst.com

@bogdansoare
Copy link

Aso having this issue, but only on production builds, on development it works as expected

@andrezimpel
Copy link

@bogdansoare yea, it's only happening in production!

@denisgoryaynov
Copy link

I have the same issue

@bogdansoare
Copy link

For now the easiest way to fix this is to have 2 gatsby images one for small screens and one for larger, then toggle between the 2 of them with media queries

@polarathene
Copy link
Contributor

Please sub to this issue to be notified of a fix, continue discussion there.

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.

8 participants