feat(gatsby-remark-images): make images blur up #7800

jamesadarich · 2018-09-01T15:47:56Z

Hey guys,

This is a proposal for adding functionality to mimic the blur up effect in gatsby-image for gatsby-remark-images. This could be nicer I think but I can't see an obvious way to do that without a great deal of refactoring around how the remark plugins are integrating (but hopefully you guys can guide to an awesome solution :) )

The pros and cons of this solution, I believe, are as follow;

Pros

in keeping with current code (should be minimal impact change)
works for images with transparencies
no need to calculate ratio

Cons

inline script - not great for CSP but I've had some thoughts about resolutions for this in general (but this is a separate piece)
reliant on class names staying the same (but I did a minor refactor to help avoid this being a problem)
not really testable (I have tested against a local gatsby solution and it seems to work cross browser :) )

Let me know what you guys think about the solution and any ideas you guys have

KyleAMathews · 2018-09-01T16:01:40Z

Looks promising!

Instead of inlining the code however (which would cause a lot of code duplication), you can add the code using gatsby-ssr.js to the <head> using onRenderBody https://next.gatsbyjs.org/docs/ssr-apis/#onRenderBody

jamesadarich · 2018-09-01T20:47:53Z

@KyleAMathews I've given this a go but under tests it would seem that gatsby hasn't loaded the markdown at the point onRenderBody adds the script so this is proving problematic. From reading the other available API docs doesn't seem to be anything that would cover this situation unless you've got any ideas? :)

KyleAMathews · 2018-09-01T21:12:46Z

This is for server rendering. Check out some of the other components that use this?

Another option is to use https://www.gatsbyjs.org/docs/browser-apis/#onRouteUpdate

jamesadarich · 2018-09-01T22:16:02Z

Newest update moves script to gatsby-browser.js as expected. Behaviour still remains the same except for internet explorer which seems to fail as the javascript isn't compiled to es5 in the bundle. Need to investigate this one a bit more though.

KyleAMathews · 2018-09-01T22:19:07Z

Oh nice! Yeah, you need to change the package's babelrc to be like this so the browser code is transpiled https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-image/.babelrc

jamesadarich · 2018-09-01T22:26:45Z

Spot on 👍 all up to date now ready for another review round :)

jamesadarich · 2018-09-04T22:16:29Z

@KyleAMathews I've removed the [WIP] tag as I think we've gotten to a good point now.

Final improvements:

if image loads form cache the effect does not show as we already have the image ready for the user
if for whatever reason the script fails there is some defensive code in there to only set the opacity of unloaded images to 0 after we know we can implement the effect

I've updated the snapshot in line with the changes so we should be ready to go barring any feedback you guys have :) let me know if you want any tweaks.

As a side note I found when using a for of loop, babel didn't seem to be transpiling this to valid es5 (it uses Symbol which is es2015) is this an issue that needs to be picked up separately?

pieh · 2018-10-24T23:18:37Z

packages/gatsby-remark-images/src/index.js

+      left: 0;
+      transition: opacity 0.5s;
+      transition-delay: 0.5s;
+      box-shadow: inset 0px 0px 0px 400px ${


Was making this verbose intentional? This inflates outputted html with extra bytes. Perhaps we can keep it in readable form but actually output compressed code?

Sure, my assumption was that this was minified down so shouldn't make a difference but much easier to understand what's going on.

pieh · 2018-11-14T21:47:36Z

packages/gatsby-remark-images/src/index.js

-    >${imageTag}</span>
+    <img
+      class="${imageBackgroundClass}"
+      style="width: 100%; height: 100%; position: relative; bottom: 0; left: 0; transition-delay: 0.5s; tranistion: opacity 0.5s; opacity: 1; background-size: cover; display: block;"


typo in transition property

I'm also not sure if it's good idea changing span to img here - there might be a11y implications if screenreader see 2 image tags instead of one for essentially same image?

Hmmm... not sure to be honest I'll take your guidance on this one. Looking at gatsby-image it seems that the actual image is wrapped in a Tag that's passed in as a prop. So would we put this back into the span and keep the background as is? As this would be similar to what's going on there.

I remember there were some discussions previously about and some potential issues with having second as placeholder - I don't remember what the resolution of the issue was at this moment. Is there any reason to actually change from current span to img?

From memory it was while I was working through a good way to implement it so went through several variations so I think this is probably just a side effect. I'll add it back in so essentially the below?

<img class="${imageBackgroundClass}" ... /> ${imageTag}

so previously it was

 <img> 

which won't work if we won't to hide placeholder that previously was in .gatsby-resp-image-background-image (as it would hide img too) maybe this:

 <img> 

to change as little as possible (limit potential regressions) in this PR?

I'll have a play around to see what works 👍 thanks for comments, appreciate it :)

pieh · 2018-11-14T21:50:15Z

packages/gatsby-remark-images/src/gatsby-browser.js

+exports.onRouteUpdate = () => {    
+    const imageWrappers = document.querySelectorAll(`.${imageWrapperClass}`)
+
+    Array.prototype.forEach.call(imageWrappers, (imageWrapper) => {


I think it would be much more readable if you would use:

const imageWrappers = Array.from(document.querySelectorAll(`.${imageWrapperClass}`)) imageWrappers.forEach(imageWrapper => {

I remember why I didn't do this. There is no polyfill currently loaded and felt probably wasn't worthwhile in this case since readability was similar. Do you think it's worth adding a polyfill or keep as is @pieh?

KyleAMathews · 2018-11-14T21:51:48Z

Yeah, IntersectionObserver is great as it's a cheap way to implement lazy loading — so definitely great but can come in a follow-up PR

jamesadarich · 2018-11-15T22:27:02Z

Hey @pieh, I've addressed all feedback apart from 2 points.

move the background to a span - the reason this needs to be an image is so that it takes up the correct space as the placeholder to avoid jumping during the page load. This is how gatsby-image achieves it so I would say this is probably OK
refactor to use Array.from - I haven't done this as the readability seems similar but probably not worth a polyfill for this one case as it's not used anywhere else in gatsby on the browser.

Let me know if there's anything else I can do or you have any questions :)

pieh · 2018-11-16T15:26:10Z

move the background to a span - the reason this needs to be an image is so that it takes up the correct space as the placeholder to avoid jumping during the page load.

But this was already working with paddingBottom which was ensuring proper aspect ratio.

This is how gatsby-image achieves it so I would say this is probably OK

That is not correct - placeholder image in gatsby-image has position: absolute - for fluid it is achieved with extra element that has paddingBottom which ensure proper aspect/ratio (and this is how it was done here as well)

Main issue here is that you are applying unneeded changes that don't fix anything and potentially cause some problems :(

2. refactor to use Array.from - I haven't done this as the readability seems similar but probably not worth a polyfill for this one case as it's not used anywhere else in gatsby on the browser.

You are right, I should have check to see if it's safe to use. Might be worth commenting there that we are not using something easier to use because we would need to polyfill it?

jamesadarich · 2018-11-16T16:11:04Z

@pieh
1.
yep, true I missed that one sorry. Just so long since I originally did that I forgot I'll sort that :)

a) any guidance on where this should be done?
b) is this going to block the PR or should be something raised as an issue to resolve separately?

pieh · 2018-11-16T17:22:25Z

a) any guidance on where this should be done?
b) is this going to block the PR or should be something raised as an issue to resolve separately?

I mean this could be just code comment right above you using it - it would be great if this would something like article we could link to (i.e. https://css-tricks.com/snippets/javascript/loop-queryselectorall-matches/ ). Which btw it seems like using classic for loop might be best comprise between readable and supported without polyfill

jamesadarich · 2018-11-16T17:59:18Z

true, I'll refactor to for of

jamesadarich · 2018-11-16T23:24:46Z

Think we're there @pieh :) not a for of but a for because internet explorer. Let me know if we need anything else but think we should be good 👍

…s actually needed

pieh

Thanks @jamesadarich!

gatsbot · 2018-11-21T17:48:12Z

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

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

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. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
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!

pieh · 2018-11-21T17:56:00Z

published [email protected]

t2ca · 2018-11-21T19:07:10Z

Would be great if someone could make gatsby-remark-images-contentful compatible with this update.

Thanks!

oorestisime · 2018-11-21T20:28:08Z

thank you all for finalizing this. @jamesadarich do you intend on having a look at lazy loading for this? i am happy to give this a go IF you don't intend to / not have time.

i don't use contentful so i won't touch that part sorry :/

KyleAMathews · 2018-11-21T20:54:08Z

This is awesome! Great work @jamesadarich!

jamesadarich · 2018-11-21T21:19:47Z

@KyleAMathews thanks, loving Gatsby glad I could give something back :)

@pieh thanks for your patience and assistance with this one 👍

@t2ca I've just had a brief look at this package and has a lot of stuff in common so probably worth sharing some of the functionality for sure. Maybe we could talk about how to do that if it's something you're keen on doing? Feel free to contact me directly :) Like with @oorestisime it's not something I use but it'd be cool to refactor the code to have more shared stuff so it's interesting but I think lazy loading for me is higher priority

@oorestisime I definitely did want to have a peek at this but maybe it's something we can collaborate on if that works for you? :)

oorestisime · 2018-11-21T21:31:16Z

nah go ahead! my intention was just to make sure this gets done. it is not urgent, for me, so go for it. (i can have a look/test it once you have something :) )

BREAKING CHANGE: html markup has been changed, high resolution image (img.gatsby-resp-image-image) is no longer nested inside span with low resolution placeholder (span.gatsby-resp-image-wrapper) - it's sibling of that span now. This might affect any custom styling that is applied to inline images

jamesadarich added 5 commits August 31, 2018 22:02

remove duplicate src

29023c9

transition opacity on image load

1533368

rework background to be a second image

98e6099

reorder image transition css

daac90e

rework into inline script

7a2debf

jamesadarich added 2 commits September 1, 2018 20:40

split out script into gatsby-ssr

8b01fec

update .gitignore with new built files

3da01c3

rework script to gatsby browser

c3c5b53

update babelrc to compile appropriately for browsers

79c1aa5

jamesadarich added 4 commits September 3, 2018 23:52

fix linting issues

513de57

update browser script to be safer

ff15235

make blur up safer and only happen if necessary

3522015

update test snapshot

10a20f8

jamesadarich changed the title ~~[WIP] Make gatsby-remark-images blur up~~ Make gatsby-remark-images blur up Sep 4, 2018

jamesadarich changed the title ~~Make gatsby-remark-images blur up~~ [gatsby-remark-images] Make images blur up Sep 4, 2018

KyleAMathews force-pushed the master branch 5 times, most recently from fc4ca3e to 2116fff Compare September 17, 2018 20:36

jamesadarich mentioned this pull request Oct 24, 2018

Should gatsby-remark-images "Blur Up"? #7781

Closed

Merge branch 'master' into issue/7781

a983e8c

pieh reviewed Oct 24, 2018

View reviewed changes

pieh reviewed Nov 14, 2018

View reviewed changes

jamesadarich added 2 commits November 15, 2018 22:12

review feedback

62157d5

swap to back ticks

1c5a4cd

format

ebb0929

jamesadarich mentioned this pull request Nov 15, 2018

gatsby-remark-images doesn't remove base64 background-image after image loading #8970

Closed

make more consistent with previous behaviour

7d03cc6

pieh added 5 commits November 21, 2018 17:01

fix loop condition and add comment why classic for is used

58b05f1

remove unneded changes, move adding transition property only when it'…

e6ce7c4

…s actually needed

update snapshot

a39a73d

format

637c9e7

trim whitespaces in markup to make output more condensed

3ab67d7

pieh changed the title ~~[gatsby-remark-images] Make images blur up~~ feat(gatsby-remark-images): make images blur up Nov 21, 2018

pieh approved these changes Nov 21, 2018

View reviewed changes

pieh merged commit 5a5254e into gatsbyjs:master Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby-remark-images): make images blur up #7800

feat(gatsby-remark-images): make images blur up #7800

jamesadarich commented Sep 1, 2018

KyleAMathews commented Sep 1, 2018

jamesadarich commented Sep 1, 2018

KyleAMathews commented Sep 1, 2018

jamesadarich commented Sep 1, 2018

KyleAMathews commented Sep 1, 2018

jamesadarich commented Sep 1, 2018

jamesadarich commented Sep 4, 2018

pieh Oct 24, 2018

jamesadarich Oct 24, 2018

pieh Nov 14, 2018

pieh Nov 14, 2018

jamesadarich Nov 14, 2018

pieh Nov 14, 2018

jamesadarich Nov 14, 2018

pieh Nov 14, 2018

jamesadarich Nov 14, 2018

pieh Nov 14, 2018

jamesadarich Nov 15, 2018

KyleAMathews commented Nov 14, 2018

jamesadarich commented Nov 15, 2018

pieh commented Nov 16, 2018 •

edited

Loading

jamesadarich commented Nov 16, 2018

pieh commented Nov 16, 2018

jamesadarich commented Nov 16, 2018

jamesadarich commented Nov 16, 2018

pieh left a comment

gatsbot bot commented Nov 21, 2018

pieh commented Nov 21, 2018

t2ca commented Nov 21, 2018

oorestisime commented Nov 21, 2018

KyleAMathews commented Nov 21, 2018

jamesadarich commented Nov 21, 2018

oorestisime commented Nov 21, 2018

feat(gatsby-remark-images): make images blur up #7800

feat(gatsby-remark-images): make images blur up #7800

Conversation

jamesadarich commented Sep 1, 2018

KyleAMathews commented Sep 1, 2018

jamesadarich commented Sep 1, 2018

KyleAMathews commented Sep 1, 2018

jamesadarich commented Sep 1, 2018

KyleAMathews commented Sep 1, 2018

jamesadarich commented Sep 1, 2018

jamesadarich commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleAMathews commented Nov 14, 2018

jamesadarich commented Nov 15, 2018

pieh commented Nov 16, 2018 • edited Loading

jamesadarich commented Nov 16, 2018

pieh commented Nov 16, 2018

jamesadarich commented Nov 16, 2018

jamesadarich commented Nov 16, 2018

pieh left a comment

Choose a reason for hiding this comment

gatsbot bot commented Nov 21, 2018

pieh commented Nov 21, 2018

t2ca commented Nov 21, 2018

oorestisime commented Nov 21, 2018

KyleAMathews commented Nov 21, 2018

jamesadarich commented Nov 21, 2018

oorestisime commented Nov 21, 2018

pieh commented Nov 16, 2018 •

edited

Loading