feat(gatsby-image): Placeholder Improvements #10944
Conversation
Allows for using larger(or smaller) than the default `20px` width, trading size for quality via more/less pixels. Adds the `base64Width` arg to the fixed & fluid nodes.
Provides added clarity for where these numbers are used. Especially for the value of `72` where it's not clear if the value was for image or display density. `defaultBase64Width` improves clarity and keeps it DRY across both usages.
Place the `backgroundColor` div before/behind the placeholder img.
They seem all intended to use the same duration of `0.25s`, `fluid` backgroundColor div actually used `0.35s` for some reason which seemed a possible typo, this corrects that and avoids it in future.
For example, WEBP base64 placeholders with a `backgroundColor` as fallback while using JPG/PNG with WEBP for the `<picture>` element. Adds the `forceBase64Format` config option. Adds support for WEBP base64 images.
Allows setting a project wide default, rather than having to request it per image. Updates `defaultBase64Width` to be a function as `pluginOptions` has not initialized when assigned.
polarathene
force-pushed
the
topic/gatsby-image-placeholder-improvents
branch
from
b100584
to
142f484
Compare
|
Could this PR receive some feedback/review please? |
|
Would a demo project to visually see the changes help at all? I could set one up? I'd appreciate receiving feedback on this and getting a decision to merge or reject as the longer this PR stays unattended the more conflicts I'll have to resolve(which I'd rather not). |
| @@ -672,13 +688,14 @@ async function fluid({ file, args = {}, reporter, cache }) { | |||
| }) | |||
| }) | |||
|
|
|||
| const base64Width = 20 | |||
| const base64Width = options.base64Width || defaultBase64Width() | |||
There was a problem hiding this comment.
can't you just use options.base64Width? It should be always set because of the healOptions function
There was a problem hiding this comment.
I am pretty sure I required it here but it's been a while since I wrote this so I'm not sure what situation required it.(EDIT: Answered)
Looking through the commits, this const was originally hardcoded with a value of 20. I then had options.base64Width || 20 before moving the default value into a variable, and finally a function(to support a config option value in gatsby.config.js, otherwise falling back to the default value of 20).
options.base64Width is an optional param that can be set on the node/graphql function, should you want to use a different base64 width than the default value of 20. If you want to override the default globally via gatsby.config.js, defaultBase64Width() will return that value instead of 20.
At line 806, base64Args for the fixed() variant, width param is replaced with the options.base64Width if provided.
It should be always set because of the healOptions function
Went over the code, the fluid() and fixed() functions initialize the options object at the start with no default params passed in. Only generateBase64() does this, where it originally passed in the hard coded default width of 20, my PR replaced that with defaultBase64Width() accordingly. That method is called by both fluid and fixed methods when they call base64() which first checks for a cached base64 data else it creates a new base64 image assigning the default width.
Fluid needs to know the width in advance for computing the height, this was the case prior to my PR as well.
Fixed derives it's height from the width during generation of base64 image afaik, I added a width param to it's base64 args only if a custom width was requested, otherwise it's provided a default width during base64 generation as mentioned above with generateBase64.
TL;DR:
No, options.base64Width isn't handled by healOptions(), it's passed in from a param in the graphql query.
For fluid() the width must be known to derive the base64Height, prior to this PR it was a fixed value, 20; now it can also be a configurable global value in gatsby.config.js too.
For fixed(), it's only interested in the graphql query param if available, otherwise it gets a default width from the 2nd healOptions() at a later stage in generateBase64()(assuming no cached version exists).
Should options.base64Width be changed to args.base64Width to avoid this confusion with healOptions()? Should any of what I've said be added as comments to document what is going on within the code more clearly?
I'm not 100% sold on this one but the user decides so I don't see a big issue with it |
wardpeet
added
the
status: awaiting author response
Co-Authored-By: polarathene <polarathene@users.noreply.github.com>
Co-Authored-By: polarathene <polarathene@users.noreply.github.com>
|
Sorry for the delay, had a long flight.
Might help to split into multiple files? It's getting a bit large in size. I can understand the hesitance, I just spent a fair bit of time trying to answer one of your review questions and had to grok the flow of logic again to understand my small changes and if I misunderstood anything based on that question.
It's not default iirc, Chrome and Firefox(since earlier this year) have WEBP support now and this PR allows for using Where to from here? If this PR looks good to merge, I'll fix the conflicts and look into updating related docs? |
wardpeet
removed
the
status: awaiting author response
@wardpeet Just to clarify, this PR only lets you customize the base64 width(height is still inferred). Thanks for taking the time to review and merge :) |
3 features:
20px. Closes [gatsby-plugin-sharp] custom base64 width and height of 1px to get 'average color' placeholder #98571 fix:
backgroundColorandbase64placeholder elements allowing both to work together. Resolves gatsby-remark-images-contentful isn't doing the "blur up" effect, just fades in #70252 refactors:
Forcing formats(base64 or current support) doesn't work reliably currently. Bug has been fixed inAvailable!sharpand available when the0.21.2release is published.Related Issues
Closes #10705 (My own issue requesting these changes and feedback to proceed)