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

feat(gatsby-image): Add art direction #13395

Merged
merged 32 commits into from
Jun 21, 2019
Merged

feat(gatsby-image): Add art direction #13395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2d269f3
Add optional media key to PropTypes and Typescript declarations
brimtown Apr 8, 2019
2ce14e0
Add optional fluidImages and fixedImages props
brimtown Apr 10, 2019
c9f2f36
Add art direction to fixed and fluid images
brimtown Apr 10, 2019
5d3ecb1
Add art direction to base64 and tracedSVG
brimtown Apr 10, 2019
79d557c
Add art direction to noscript image
brimtown Apr 16, 2019
cf3c58c
Add tests for fixedImages and fluidImages
brimtown Apr 16, 2019
46dc731
Merge remote-tracking branch 'upstream/master' into feat/add-image-ar…
pieh Apr 16, 2019
6282c75
Respond to code review
brimtown Apr 30, 2019
da6b873
Merge remote-tracking branch 'upstream/master' into feat/add-image-ar…
brimtown Apr 30, 2019
fc12f62
Use const in tests
brimtown Apr 30, 2019
03226cb
Merge remote-tracking branch 'upstream/master' into feat/add-image-ar…
brimtown May 16, 2019
5317331
Merge branch 'feat/add-image-art-direction' of github.com:brimtown/ga…
brimtown May 16, 2019
f283cad
Merge remote-tracking branch 'upstream/master' into feat/add-image-ar…
brimtown May 16, 2019
e81b2b7
Additinal code review refactor
brimtown May 23, 2019
57363a5
Merge remote-tracking branch 'upstream/master' into feat/add-image-ar…
brimtown May 23, 2019
a58da9b
Fix e2e tests
brimtown May 27, 2019
a6d966d
Merge remote-tracking branch 'upstream/master' into feat/add-image-ar…
brimtown May 27, 2019
6f5bb09
Add README docs
brimtown May 28, 2019
b685421
Fix typo and update wording in README
brimtown May 28, 2019
4ba3b86
Name selectors in e2e test
brimtown May 30, 2019
c98c23d
Work around SVG bug by encoding spaces
brimtown Jun 11, 2019
5ceebb9
Fix breaking Placeholder change, respond to code review, and update s…
brimtown Jun 11, 2019
85c844a
Use @polarthene's Pastebin
brimtown Jun 11, 2019
d7b102a
Update sharp snapshot test
brimtown Jun 11, 2019
55063e3
Reset integration tests
brimtown Jun 11, 2019
91478a2
Merge branch 'master' into pr/13395
wardpeet Jun 17, 2019
9aa56b1
Move fluidImages & fixedImages into fluid & fixed
wardpeet Jun 17, 2019
a0d0c78
update tests with no media
wardpeet Jun 17, 2019
c298cc6
cleanup spreadprops
wardpeet Jun 17, 2019
9895126
Add warning if multiple sources with no media were used
wardpeet Jun 17, 2019
6257cdf
review changes
wardpeet Jun 18, 2019
836ce77
fix tests
wardpeet Jun 21, 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
27 changes: 7 additions & 20 deletions examples/using-gatsby-image/src/components/floating-image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,6 @@ const Image = styled(Img)`
margin-left: ${rhythm(options.blockMarginBottom * 2)};
margin-right: -${gutter.default};

${mq.phablet} {
display: none;
}
`

const ImageDesktop = styled(Image)`
display: none;

${mq.phablet} {
display: block;
}

${mq.tablet} {
margin-right: -${gutter.tablet};
}
Expand Down Expand Up @@ -53,15 +41,14 @@ const FloatingImage = ({
https://www.gatsbyjs.org/packages/gatsby-image/#gatsby-image-props
*/}
<Image
fixed={imageMobile}
backgroundColor={backgroundColor ? backgroundColor : false}
style={{ display: `inherit` }}
title={title}
/>
<ImageDesktop
fixed={imageDesktop}
fixed={[
imageMobile,
{
...imageDesktop,
media: mq.phablet.replace(`@media`, ``).trim(),
},
]}
backgroundColor={backgroundColor ? backgroundColor : false}
style={{ display: `inherit` }}
title={title}
/>
</React.Fragment>
Expand Down
59 changes: 28 additions & 31 deletions packages/gatsby-image/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ You will need to add it in your graphql query as is shown in the following snipp

## Art-directing multiple images

`gatsby-image` supports showing different images at different breakpoints, which is known as [art direction](https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images#Art_direction). To do this, you can define your own array of `fixed` or `fluid` images, along with a `media` key per image, and pass it to `gatsby-image`'s `fixedImages` or `fluidImages` props. The `media` key that is set on an image can be any valid CSS media query.
`gatsby-image` supports showing different images at different breakpoints, which is known as [art direction](https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images#Art_direction). To do this, you can define your own array of `fixed` or `fluid` images, along with a `media` key per image, and pass it to `gatsby-image`'s `fixed` or `fluid` props. The `media` key that is set on an image can be any valid CSS media query.

```jsx
import React from "react"
Expand All @@ -344,10 +344,7 @@ export default ({ data }) => {
// Set up the array of image data and `media` keys.
// You can have as many entries as you'd like.
const sources = [
{
...data.mobileImage.childImageSharp.fluid,
media: `(max-width: 767px)`,
},
data.mobileImage.childImageSharp.fluid,
{
...data.desktopImage.childImageSharp.fluid,
media: `(min-width: 768px)`,
Expand All @@ -357,7 +354,7 @@ export default ({ data }) => {
return (
<div>
<h1>Hello art-directed gatsby-image</h1>
<Img fluidImages={sources} />
<Img fluid={sources} />
</div>
)
}
Expand Down Expand Up @@ -388,31 +385,31 @@ While you could achieve a similar effect with plain CSS media queries, `gatsby-i

## `gatsby-image` props

| Name | Type | Description |
| ---------------------- | ------------------- | ---------------------------------------------------------------------------------------------------------------------------- |
| `fixed` | `object` | Data returned from the `fixed` query |
| `fluid` | `object` | Data returned from the `fluid` query |
| `fadeIn` | `bool` | Defaults to fading in the image on load |
| `durationFadeIn` | `number` | fading duration is set up to 500ms by default |
| `title` | `string` | Passed to the `img` element |
| `alt` | `string` | Passed to the `img` element. Defaults to an empty string `alt=""` |
| `crossOrigin` | `string` | Passed to the `img` element |
| `className` | `string` / `object` | Passed to the wrapper element. Object is needed to support Glamor's css prop |
| `style` | `object` | Spread into the default styles of the wrapper element |
| `imgStyle` | `object` | Spread into the default styles of the actual `img` element |
| `placeholderStyle` | `object` | Spread into the default styles of the placeholder `img` element |
| `placeholderClassName` | `string` | A class that is passed to the placeholder `img` element |
| `backgroundColor` | `string` / `bool` | Set a colored background placeholder. If true, uses "lightgray" for the color. You can also pass in any valid color string. |
| `onLoad` | `func` | A callback that is called when the full-size image has loaded. |
| `onStartLoad` | `func` | A callback that is called when the full-size image starts loading, it gets the parameter { wasCached: boolean } provided. |
| `onError` | `func` | A callback that is called when the image fails to load. |
| `Tag` | `string` | Which HTML tag to use for wrapping elements. Defaults to `div`. |
| `objectFit` | `string` | Passed to the `object-fit-images` polyfill when importing from `gatsby-image/withIEPolyfill`. Defaults to `cover`. |
| `objectPosition` | `string` | Passed to the `object-fit-images` polyfill when importing from `gatsby-image/withIEPolyfill`. Defaults to `50% 50%`. |
| `loading` | `string` | Set the browser's native lazy loading attribute. One of `lazy`, `eager` or `auto`. Defaults to `lazy`. |
| `critical` | `bool` | Opt-out of lazy-loading behavior. Defaults to `false`. Deprecated, use `loading` instead. |
| `fixedImages` | `array` | An array of objects returned from `fixed` queries. When combined with `media` keys, allows for art directing `fixed` images. |
| `fluidImages` | `array` | An array of objects returned from `fluid` queries. When combined with `media` keys, allows for art directing `fluid` images. |
| Name | Type | Description |
| ---------------------- | ------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
| `fixed` | `object` / `array` | Data returned from the `fixed` query. When prop is an array it has to be combined with `media` keys, allows for art directing `fixed` images. |
| `fluid` | `object` / `array` | Data returned from the `fluid` query. When prop is an array it has to be combined with `media` keys, allows for art directing `fluid` images. |
| `fadeIn` | `bool` | Defaults to fading in the image on load |
| `durationFadeIn` | `number` | fading duration is set up to 500ms by default |
| `title` | `string` | Passed to the `img` element |
| `alt` | `string` | Passed to the `img` element. Defaults to an empty string `alt=""` |
| `crossOrigin` | `string` | Passed to the `img` element |
| `className` | `string` / `object` | Passed to the wrapper element. Object is needed to support Glamor's css prop |
| `style` | `object` | Spread into the default styles of the wrapper element |
| `imgStyle` | `object` | Spread into the default styles of the actual `img` element |
| `placeholderStyle` | `object` | Spread into the default styles of the placeholder `img` element |
| `placeholderClassName` | `string` | A class that is passed to the placeholder `img` element |
| `backgroundColor` | `string` / `bool` | Set a colored background placeholder. If true, uses "lightgray" for the color. You can also pass in any valid color string. |
| `onLoad` | `func` | A callback that is called when the full-size image has loaded. |
| `onStartLoad` | `func` | A callback that is called when the full-size image starts loading, it gets the parameter { wasCached: boolean } provided. |
| `onError` | `func` | A callback that is called when the image fails to load. |
| `Tag` | `string` | Which HTML tag to use for wrapping elements. Defaults to `div`. |
| `objectFit` | `string` | Passed to the `object-fit-images` polyfill when importing from `gatsby-image/withIEPolyfill`. Defaults to `cover`. |
| `objectPosition` | `string` | Passed to the `object-fit-images` polyfill when importing from `gatsby-image/withIEPolyfill`. Defaults to `50% 50%`. |
| `loading` | `string` | Set the browser's native lazy loading attribute. One of `lazy`, `eager` or `auto`. Defaults to `lazy`. |
| `critical` | `bool` | Opt-out of lazy-loading behavior. Defaults to `false`. Deprecated, use `loading` instead. |
| `fixedImages` | `array` | An array of objects returned from `fixed` queries. When combined with `media` keys, allows for art directing `fixed` images. |
| `fluidImages` | `array` | An array of objects returned from `fluid` queries. When combined with `media` keys, allows for art directing `fluid` images. |

## Image processing arguments

Expand Down
4 changes: 2 additions & 2 deletions packages/gatsby-image/src/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ const setupImages = (
title={`Title for the image`}
alt={`Alt text for the image`}
crossOrigin={`anonymous`}
{...fluidImages && { fluidImages: fluidImagesShapeMock }}
{...!fluidImages && { fixedImages: fixedImagesShapeMock }}
{...fluidImages && { fluid: fluidImagesShapeMock }}
{...!fluidImages && { fixed: fixedImagesShapeMock }}
onLoad={onLoad}
onError={onError}
itemProp={`item-prop-for-the-image`}
Expand Down
31 changes: 13 additions & 18 deletions packages/gatsby-image/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,17 @@ const convertProps = props => {
convertedProps.loading = `eager`
}

// convert fluid & fixed to arrays so we only have to work with arrays
convertedProps.fluid = [].concat(convertedProps.fluid).filter(Boolean)
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
convertedProps.fixed = [].concat(convertedProps.fixed).filter(Boolean)

return convertedProps
}

// Find the source of an image to use as a key in the image cache.
// Use `fixed` or `fluid` if specified, or the first image in
// either `fixedImages` or `fluidImages`
const getImageSrcKey = ({ fluid, fixed, fluidImages, fixedImages }) => {
const data =
fluid ||
fixed ||
(fluidImages && fluidImages[0]) ||
(fixedImages && fixedImages[0])
// Use `the first image in either `fixed` or `fluid`
const getImageSrcKey = ({ fluid, fixed }) => {
const data = (fluid.length && fluid[0]) || (fixed.length && fixed[0])
Copy link
Contributor

@polarathene polarathene Jun 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is the length check necessary? It'll likely fail iirc checking the length property if the prop itself is undefined? Or is this because of convertProps() that it's safe to always assume an array value present?

I would add that we have both inImageCache() and activateCacheForImage() both calling this method after first doing:

const convertedProps = convertProps(props)

We might as well just move that duplication into this method and drop the need to pass in props?

Suggested change
const data = (fluid.length && fluid[0]) || (fixed.length && fixed[0])
const convertedProps = convertProps(props)
const data = (fluid.length && fluid[0]) || (fixed.length && fixed[0])

EDIT: I see that your convertProps() method returns an array regardless, so if it's empty a 0 falsey value for fluid will select fixed instead. My bad :)

return data.src
}

Expand Down Expand Up @@ -356,8 +355,6 @@ class Image extends React.Component {
placeholderClassName,
fluid,
fixed,
fluidImages,
fixedImages,
backgroundColor,
durationFadeIn,
Tag,
Expand Down Expand Up @@ -395,8 +392,8 @@ class Image extends React.Component {
className: placeholderClassName,
}

if (fluid || fluidImages) {
const imageVariants = fluidImages ? groupByMedia(fluidImages) : [fluid]
if (fluid.length) {
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
const imageVariants = groupByMedia(fluid)
polarathene marked this conversation as resolved.
Show resolved Hide resolved
const image = imageVariants[0]

return (
Expand Down Expand Up @@ -494,8 +491,8 @@ class Image extends React.Component {
)
}

if (fixed || fixedImages) {
const imageVariants = fixedImages ? groupByMedia(fixedImages) : [fixed]
if (fixed.length) {
const imageVariants = groupByMedia(fixed)
const image = imageVariants[0]

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that in the fixed section, there is code you've not handled which references image. divStyle, bgColor and the Img tag(although I can't recall the impact of width/height attributes defined on an img element, have you confirmed variants with different width/height as fixed elements render correctly?

Adjusting the width/height attributes on the fixed image in this demo doesn't appear to have any effect, so I guess it's just the other two that should be taken into consideration?

fluid section also references the images aspectRatio for the paddingBottom style. That's a case which may have gone missed?

const divStyle = {
Expand Down Expand Up @@ -634,10 +631,8 @@ const fluidObject = PropTypes.shape({
Image.propTypes = {
resolutions: fixedObject,
sizes: fluidObject,
fixed: fixedObject,
fluid: fluidObject,
fixedImages: PropTypes.arrayOf(fixedObject),
fluidImages: PropTypes.arrayOf(fluidObject),
fixed: PropTypes.oneOfType([fixedObject, PropTypes.arrayOf(fixedObject)]),
fluid: PropTypes.oneOfType([fluidObject, PropTypes.arrayOf(fluidObject)]),
fadeIn: PropTypes.bool,
durationFadeIn: PropTypes.number,
title: PropTypes.string,
Expand Down