added: rootMargin prop that is a user override for the default 200px

[grantglidewell]

Description

Image component takes a prop that allows the user to override the default 200px rootMargin.

Related Issues

Addresses #13815

[lannonbr]

@grantglidewell Can you also update the README for gatsby-image to also show that this new prop exists on the component?

[grantglidewell]

@grantglidewell Can you also update the README for gatsby-image to also show that this new prop exists on the component?

I looked at the README and realized the <Image /> component isn't covered, only <Img />. I'll see what I can come up with for it.

[lannonbr]

It's the same component @grantglidewell. Doesn't really matter what is called when a user imports it

[DSchau]

Looking great! Few things:

1. Would recommend adding a defaultProp rather than a fallback within the component
2. Would recommend making the prop more flexible, e.g. if 200 is passed, we should presume 200px
3. Would strongly encourage adding a few unit tests (__tests__/index.js)

`const rootMargin = typeof this.props.rootMargin === 'number' ? `${this.props.rootMargin}px` : this.props.rootMargin`

Thanks for this!

[pieh]

Can we call it something more descriptive? rootMargin is property of intersection observer API, and I tbh I wasn't sure what it does until I dived into code changes here.

Maybe prop name itself is ok (I can't really think of better name), but documentation should be expanded on what it does (like how far away component need to be from viewport to start lazy loading)

[grantglidewell]

Can we call it something more descriptive? rootMargin is property of intersection observer API, and I tbh I wasn't sure what it does until I dived into code changes here.

Maybe prop name itself is ok (I can't really think of better name), but documentation should be expanded on what it does (like how far away component need to be from viewport to start lazy loading)

Ill update the documentation so its clearer

[KyleAMathews]

Curious why you want to modify this? Ideally we don't add new props unless absolutely required.

[pieh]

There is also TypeScript definition file that should be updated with the new prop ( https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-image/index.d.ts#L25-L45 ) - if You are not familiar with TS, that's ok we will manage it (just mentioning it to not forget about it)

[KyleAMathews]

E.g. if 200px isn't enough, we could modify it for everyone.

[lannonbr]

@KyleAMathews The idea was that someone may want to increase the bounding box to trigger the image to load in so the image could be further down the page and could load in before it is even on the screen

[KyleAMathews]

Right but why would this vary across sites?

We don't want to add options unless there's deep differences between sites that we can't automatically determine.

One of the guiding design principles of gatsby is that performance is automatic.

[grantglidewell]

Right but why would this vary across sites?

We don't want to add options unless there's deep differences between sites that we can't automatically determine.

One of the guiding design principles of gatsby is that performance is automatic.

Since the default prop is the same, this doesnt add any configuration, just additional optional control to those who may want it. This seems like just an escape hatch

[KyleAMathews]

A similar case is the new native image loading. They explicitly do not give you control over when lazy loading starts as they want to be able to control that and optimize it over time. This feels like a similar case.

Also, given image lazy loading is shipping in chrome and presumably in other browsers soon, which we'll default to using instead of intersection observer, this proposal has a limited shelf life.

I'd prefer we'd redirect energy to finding better ways to know when to start lazy loading if this is an issue.

[timhagn]

Should this be merged or not, thanks for bringing it up! Cause over at gatsby-background-image setting the rootMargin to a default of 200px might interfere with lazy-loading of full screen (100vh height) images, or do I err? Should this be the case and you be willing to integrate it over there, @grantglidewell, I'd welcome it. Although on reading @KyleAMathews last comment, I'm unsure on how to proceed, too...

[grantglidewell]

The jest snapshots are passing for me locally (failing CI), any suggestions?

[timhagn]

@grantglidewell The only difference I see between your tests and the ones before is that you give durationFadeIn as a Number instead of a String (though it has a PropType of Number oO). But all transitions are missing from the snapshot tests beforehand...

[grantglidewell]

@timhagn seems weird for sure. Either way Ill wait to hear if this should move forward before putting more work into it.

lmk if I should move forward @KyleAMathews

[Oliweer]

The best way would be to have a global option that could be accessed programatically, an idea that i've had is that once a user access your site you increase the rootMargin so that the expierence of the site feels better. @KyleAMathews I think this is an issue with user experience as it feels weird browsing a a site with large images using the gatsby-image, you mostly get a blurry image response while scrolling, increasing this to 100vh would improve it but the issue is that the images loads too late

[KyleAMathews]

Let's not move forward with this. Once #13217 is done, setting the rootMargin won't have any effect any more as loading images will be browser controlled.

@Oliweer would love to see people experimenting with different values other than 200px — if we decide a different value works better, we can definitely change it to something else.

This is again what browsers will be doing.

[KyleAMathews]

Thanks @grantglidewell for proposing this and putting up a PR!