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 tracedSVG instead of blur up effect #9490

Merged
merged 15 commits into from
Feb 22, 2019

Conversation

tlent
Copy link
Contributor

@tlent tlent commented Oct 28, 2018

Closes #9414, #4624

Unfortunately due to #4510 the effect cannot currently be seen unless { backgroundColor: "transparent" } is set in the gatsby-remark-images options. I will try looking into that issue soon.

Please let me know if any changes need to be made. This is my first contribution to Gatsby.

@tlent
Copy link
Contributor Author

tlent commented Oct 28, 2018

#7800 seems like it will be fixing the issue with the effect not being visible unless {backgroundColor: "transparent" } is set.

If that PR is merged this PR should be changed to putplaceholderImageData inside of <img src="${placeholderImageData}" instead of <span style="background-image: url(&quot;${placeholderImageData}&quot;)">. The double quote from the style attribute was forcing the use of &quot; but with the img tag I believe it will no longer be necessary. Single quotes around placeholderImageData were not an option due to the way the traceSVG SVG data is encoded.

@tlent tlent closed this Nov 2, 2018
@tlent tlent reopened this Nov 2, 2018
@tlent tlent force-pushed the gatsby-remark-images/add-tracedsvg branch from 547c436 to ad9fbd8 Compare November 2, 2018 01:06
@tlent
Copy link
Contributor Author

tlent commented Nov 2, 2018

Sorry for closing and reopening this. That was an attempt (unsuccessful) to get the CI checks to run again since they seem to keep timing out. Not sure what the issue is with the checks.

@tlent
Copy link
Contributor Author

tlent commented Nov 24, 2018

I have now merged master including the changes from #7800. The issue where { backgroundColor: "transparent" } was required to show the preview image has been fixed, so this PR is now working perfectly.

#7800 was changed back to using <span> so my previous comment about changing the quotes no longer applies.

This PR should be ready to merge now.

style="padding-bottom: ${ratio}; position: relative; bottom: 0; left: 0; background-image: url('${
fluidResult.base64
}'); background-size: cover; display: block;"
style="padding-bottom: ${ratio}; position: relative; bottom: 0; left: 0; background-image: url(&quot;${placeholderImageData}&quot;); background-size: cover; display: block;"
Copy link

Choose a reason for hiding this comment

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

&quot; I don't think you can use a html entity here.

Copy link
Contributor Author

@tlent tlent Dec 6, 2018

Choose a reason for hiding this comment

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

This is the only way I have found to use the optimized SVG that traceSVG returns within the style attribute. Based on this stackoverflow post and the HTML spec I believe the HTML entity is valid anywhere in an HTML document. It has worked as far as I have seen in my testing.

Using double quotes around the SVG data conflicts with the style attribute's quotes. Using single quotes around the SVG data conflicts with the single quotes in the optimized SVG encoding. Using single quotes for the style attribute as a whole with double quotes around the data also conflicts with the single quotes in the optimized SVG encoding. Using no quotes around the SVG data does not work (I believe because of spaces in the data). I have not thought of any other possible solution but I am open to suggestions.

Copy link

@floo51 floo51 Dec 6, 2018

Choose a reason for hiding this comment

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

Thanks for the deep explanation.

What about escaping singles quotes in the SVG image?

const escapeSingleQuotes = (value) => value.replace(/'/g, '\\\'');
<div style="
    background-image: url('${escapeSingleQuotes(placeholderImageData)}');
    background-size: cover;
"
></div>

IMHO, it makes the code more self explanatory.

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 suggestion. It definitely is a cleaner solution. I just pushed a commit along these lines. Apparently webpack or something else is optimizing it along the way, so to make things even better there was no increase in the number of bytes sent to the browser even with the escaped quotes.

@humphreybc
Copy link
Contributor

Would be great to get this merged!

@baobabKoodaa
Copy link
Contributor

I would love to see this approved as well!

@tlent tlent requested a review from a team as a code owner February 22, 2019 14:01
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.

Verified and works as expected. Thanks! And sorry @tlent for looong wait on this.

@pieh pieh merged commit 072bcdd into gatsbyjs:master Feb 22, 2019
@sidharthachatterjee
Copy link
Contributor

Published in [email protected]

@baobabKoodaa
Copy link
Contributor

Works nicely!

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.

6 participants