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.

Sign up for GitHub

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

Jump to bottom

feat(gatsby-image): Placeholder Improvements #10944

Merged
merged 11 commits into from
Mar 12, 2019
Merged

feat(gatsby-image): Placeholder Improvements #10944

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5865ef1
feat(gatsby-plugin-sharp): Configurable base64 width
Dec 30, 2018
241c2f6
chore(gatsby-plugin-sharp): Remove magic numbers
Dec 30, 2018
34bc962
fix(gatsby-image): Support placeholder image with backgroundColor
Jan 1, 2019
43c971f
chore(gatsby-image): Share common transitionDelay variable
Jan 1, 2019
b06b9e0
feat(gatsby-plugin-sharp): Allow base64 format to differ, allow WEBP
Jan 9, 2019
c12ec40
chore(gatsby-image): Update test snapshot
Jan 9, 2019
5a62ede
feat(gatsby-plugin-sharp): Support base64Width as a config option
polarathene Jan 14, 2019
142f484
feat(gatsby-plugin-sharp): Support forceBase64Format as query arg
Jan 15, 2019
ec0071b
Change `density_72PPI` to `defaultImagePPI`
wardpeet Mar 4, 2019
7d961da
Change `density_72PPI` to `defaultImagePPI` 2
wardpeet Mar 4, 2019
66d64d3
Merge branch 'master' into pr/10944
wardpeet Mar 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@ exports[`<Img /> should render fixed size images 1`] = `
class="fixedImage gatsby-image-wrapper"
style="position: relative; overflow: hidden; display: inline; width: 100px; height: 100px;"
>
<div
style="width: 100px; opacity: 1; height: 100px;"
title="Title for the image"
/>
<img
alt=""
class="placeholder"
src="string_of_base64"
style="position: absolute; top: 0px; left: 0px; width: 100%; height: 100%; opacity: 1; color: red;"
title="Title for the image"
/>
<div
style="width: 100px; opacity: 1; height: 100px;"
title="Title for the image"
/>
<picture>
<source
srcset="some srcSetWebp"
Expand Down Expand Up @@ -50,17 +50,17 @@ exports[`<Img /> should render fluid images 1`] = `
<div
style="width: 100%; padding-bottom: 66.66666666666667%;"
/>
<div
style="position: absolute; top: 0px; bottom: 0px; opacity: 1; right: 0px; left: 0px;"
title="Title for the image"
/>
<img
alt=""
class="placeholder"
src="string_of_base64"
style="position: absolute; top: 0px; left: 0px; width: 100%; height: 100%; opacity: 1; color: red;"
title="Title for the image"
/>
<div
style="position: absolute; top: 0px; bottom: 0px; opacity: 1; right: 0px; left: 0px;"
title="Title for the image"
/>
<picture>
<source
sizes="(max-width: 600px) 100vw, 600px"
Expand Down
47 changes: 24 additions & 23 deletions packages/gatsby-image/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,11 @@ class Image extends React.Component {
const bgColor =
typeof backgroundColor === `boolean` ? `lightgray` : backgroundColor

const initialDelay = `0.25s`
const imagePlaceholderStyle = {
opacity: this.state.imgLoaded ? 0 : 1,
transition: `opacity 0.5s`,
transitionDelay: this.state.imgLoaded ? `0.5s` : `0.25s`,
transitionDelay: this.state.imgLoaded ? `0.5s` : initialDelay,
...imgStyle,
...placeholderStyle,
}
Expand Down Expand Up @@ -275,16 +276,6 @@ class Image extends React.Component {
}}
/>

{/* Show the blurry base64 image. */}
{image.base64 && (
<Img src={image.base64} {...placeholderImageProps} />
)}

{/* Show the traced SVG image. */}
{image.tracedSVG && (
<Img src={image.tracedSVG} {...placeholderImageProps} />
)}

{/* Show a solid background color. */}
{bgColor && (
<Tag
Expand All @@ -295,13 +286,23 @@ class Image extends React.Component {
top: 0,
bottom: 0,
opacity: !this.state.imgLoaded ? 1 : 0,
transitionDelay: `0.35s`,
transitionDelay: initialDelay,
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
right: 0,
left: 0,
}}
/>
)}

{/* Show the blurry base64 image. */}
{image.base64 && (
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
<Img src={image.base64} {...placeholderImageProps} />
)}

{/* Show the traced SVG image. */}
{image.tracedSVG && (
<Img src={image.tracedSVG} {...placeholderImageProps} />
)}

{/* Once the image is visible (or the browser doesn't support IntersectionObserver), start downloading the image */}
{this.state.isVisible && (
<picture>
Expand Down Expand Up @@ -361,16 +362,6 @@ class Image extends React.Component {
ref={this.handleRef}
key={`fixed-${JSON.stringify(image.srcSet)}`}
>
{/* Show the blurry base64 image. */}
{image.base64 && (
<Img src={image.base64} {...placeholderImageProps} />
)}

{/* Show the traced SVG image. */}
{image.tracedSVG && (
<Img src={image.tracedSVG} {...placeholderImageProps} />
)}

{/* Show a solid background color. */}
{bgColor && (
<Tag
Expand All @@ -379,12 +370,22 @@ class Image extends React.Component {
backgroundColor: bgColor,
width: image.width,
opacity: !this.state.imgLoaded ? 1 : 0,
transitionDelay: `0.25s`,
transitionDelay: initialDelay,
height: image.height,
}}
/>
)}

{/* Show the blurry base64 image. */}
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
{image.base64 && (
<Img src={image.base64} {...placeholderImageProps} />
)}

{/* Show the traced SVG image. */}
{image.tracedSVG && (
<Img src={image.tracedSVG} {...placeholderImageProps} />
)}

{/* Once the image is visible, start downloading the image */}
{this.state.isVisible && (
<picture>
Expand Down
26 changes: 23 additions & 3 deletions packages/gatsby-plugin-sharp/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ exports.setBoundActionCreators = actions => {

/// Plugin options are loaded onPreInit in gatsby-node
const pluginDefaults = {
forceBase64Format: false,
useMozJpeg: process.env.GATSBY_JPEG_ENCODER === `MOZJPEG`,
stripMetadata: true,
}
Expand Down Expand Up @@ -92,6 +93,7 @@ const generalArgs = {
duotone: false,
pathPrefix: ``,
toFormat: ``,
toFormatBase64: ``,
sizeByPixelDensity: false,
}

Expand All @@ -101,6 +103,7 @@ const healOptions = (args, defaultArgs) => {
options.pngCompressionLevel = parseInt(options.pngCompressionLevel, 10)
options.pngCompressionSpeed = parseInt(options.pngCompressionSpeed, 10)
options.toFormat = options.toFormat.toLowerCase()
options.toFormatBase64 = options.toFormatBase64.toLowerCase()

// only set width to 400 if neither width nor height is passed
if (options.width === undefined && options.height === undefined) {
Expand Down Expand Up @@ -475,8 +478,10 @@ function queueImageResizing({ file, args = {}, reporter }) {
}
}

// A value in pixels(Int)
const defaultBase64Width = () => pluginOptions.base64Width || 20
async function generateBase64({ file, args, reporter }) {
const options = healOptions(args, { width: 20 })
const options = healOptions(args, { width: defaultBase64Width() })
let pipeline
try {
pipeline = sharp(file.absolutePath).rotate()
Expand All @@ -485,6 +490,12 @@ async function generateBase64({ file, args, reporter }) {
return null
}

const forceBase64Format =
args.toFormatBase64 || pluginOptions.forceBase64Format
if (forceBase64Format) {
args.toFormat = forceBase64Format
}

pipeline
.resize(options.width, options.height, {
position: options.cropFocus,
Expand All @@ -499,6 +510,10 @@ async function generateBase64({ file, args, reporter }) {
progressive: options.jpegProgressive,
force: args.toFormat === `jpg`,
})
.webp({
quality: options.quality,
force: args.toFormat === `webp`,
})

// grayscale
if (options.grayscale) {
Expand Down Expand Up @@ -570,9 +585,10 @@ async function fluid({ file, args = {}, reporter, cache }) {
}

const { width, height, density, format } = metadata
const density_72PPI = 72 // Standard digital image pixel density
polarathene marked this conversation as resolved.
Show resolved Hide resolved
const pixelRatio =
options.sizeByPixelDensity && typeof density === `number` && density > 0
? density / 72
? density / density_72PPI
polarathene marked this conversation as resolved.
Show resolved Hide resolved
: 1

// if no maxWidth is passed, we need to resize the image based on the passed maxHeight
Expand Down Expand Up @@ -672,13 +688,14 @@ async function fluid({ file, args = {}, reporter, cache }) {
})
})

const base64Width = 20
const base64Width = options.base64Width || defaultBase64Width()
Copy link
Contributor

Choose a reason for hiding this comment

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

can't you just use options.base64Width? It should be always set because of the healOptions function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

const base64Height = Math.max(1, Math.round((base64Width * height) / width))
const base64Args = {
duotone: options.duotone,
grayscale: options.grayscale,
rotate: options.rotate,
toFormat: options.toFormat,
toFormatBase64: options.toFormatBase64,
width: base64Width,
height: base64Height,
}
Expand Down Expand Up @@ -787,10 +804,13 @@ async function fixed({ file, args = {}, reporter, cache }) {
})

const base64Args = {
// height is adjusted accordingly with respect to the aspect ratio
width: options.base64Width,
duotone: options.duotone,
grayscale: options.grayscale,
rotate: options.rotate,
toFormat: options.toFormat,
toFormatBase64: options.toFormatBase64,
}

// Get base64 version
Expand Down
14 changes: 14 additions & 0 deletions packages/gatsby-transformer-sharp/src/extend-node-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ const fixedNodeType = ({
height: {
type: GraphQLInt,
},
base64Width: {
type: GraphQLInt,
},
jpegProgressive: {
type: GraphQLBoolean,
defaultValue: true,
Expand Down Expand Up @@ -143,6 +146,10 @@ const fixedNodeType = ({
type: ImageFormatType,
defaultValue: ``,
},
toFormatBase64: {
type: ImageFormatType,
defaultValue: ``,
},
cropFocus: {
type: ImageCropFocusType,
defaultValue: sharp.strategy.attention,
Expand Down Expand Up @@ -241,6 +248,9 @@ const fluidNodeType = ({
maxHeight: {
type: GraphQLInt,
},
base64Width: {
type: GraphQLInt,
},
grayscale: {
type: GraphQLBoolean,
defaultValue: false,
Expand Down Expand Up @@ -269,6 +279,10 @@ const fluidNodeType = ({
type: ImageFormatType,
defaultValue: ``,
},
toFormatBase64: {
type: ImageFormatType,
defaultValue: ``,
},
cropFocus: {
type: ImageCropFocusType,
defaultValue: sharp.strategy.attention,
Expand Down