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

Moves responsive image CSS back into attribute #19888

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

fionawhim
Copy link
Contributor

Description

This PR reverts a part of #14816 that moved the image styles into a <style>
tag in the head. Per this comment from @JoshuaWalsh, putting the styles
in the was just a cleanliness improvement, and did not affect the
functionality that #14816 was addressing:

#14816 (comment)

Reverting this PR allows the responsive images to display correctly when
the Remark HTML is displayed outside of the Gatsby layout, such as in
an RSS feed.

Related Issues

Fixes #17593

Fixes gatsbyjs#17593

This PR reverts a part of gatsbyjs#14816 that moved the image styles into a <style>
tag in the head. Per this comment from @JoshuaWalsh, putting the styles
in the <head> was just a cleanliness improvement, and did not affect the
functionality that gatsbyjs#14816 was addressing:

gatsbyjs#14816 (comment)

Reverting this PR allows the responsive images to display correctly when
the Remark HTML is displayed outside of the Gatsby layout, such as in
an RSS feed.
@fionawhim fionawhim requested a review from a team as a code owner November 30, 2019 18:01
@pvdz
Copy link
Contributor

pvdz commented Dec 2, 2019

Ah sounds like a good fix :)

One semantic change is that before it was creating a css class that could apply to multiple elements. Afterwards this code is duplicated if multiple elements want to use it.

I understand if this image is never intended to be re-used by multiple elements, it's just not clear in this PR. Can you confirm please?

vertical-align: middle;
position: absolute;
top: 0;
left: 0;`.replace(/\s*(\S+:)\s*/g, `$1`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this just .replace(/\s*/g, '') ? Or are we defensive about future updates where certain rules may be whitespace sensitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also, if you're stripping whitespace anyways, it's probably nice to have the closing backtick on the next line, aligned with the const.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!

Not sure about the regexp. I copied it in from the older version of the file. I think being defensive here is a good move, though.

Since the regexp only strips before the CSS property name and between the colon and the value, it doesn’t strip the newline after the last line, so leaving backtick as-is.

@vladar
Copy link
Contributor

vladar commented Dec 5, 2019

Looks good to me. But tests are failing. You will have to re-run them with --updateSnapshot.

@fionawhim
Copy link
Contributor Author

@pvdz I can’t say if anyone has started to put gatsby-resp-image-image as a class on other elements not generated by this plugin between July when the <style> tag was first added and now. This would be a breaking change for those people, I suppose.

@vladar Thanks for the review! I updated the snapshots and pushed to this branch.

Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

oki :)

@pvdz pvdz added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 9, 2019
@gatsbybot gatsbybot merged commit 6b67d30 into gatsbyjs:master Dec 9, 2019
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.

[gatsby-remark-images] Large blank spaces / blurs in RSS feeds
4 participants