-
Notifications
You must be signed in to change notification settings - Fork 326
Generate a default srcset for an image returned by the Shopify CDN #1330
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.
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
Changes from 11 commits
b4a1d9e
f35a59d
e974e35
a809142
5fe1b7a
3c3dae8
076f666
1c559d2
c40c48f
816ed5e
65359e7
859b01b
36cc8a1
44fe410
cb84735
ce2e338
c3a2a2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@shopify/hydrogen': patch | ||
| --- | ||
|
|
||
| [#1245] - Generate a default srcset for an image returned by the Shopify CDN on the Image component and allow using a custom set of `widths.` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| import * as React from 'react'; | ||
| import {getShopifyImageDimensions, shopifyImageLoader} from '../../utilities'; | ||
| import { | ||
| getShopifyImageDimensions, | ||
| shopifyImageLoader, | ||
| addImageSizeParametersToUrl, | ||
| } from '../../utilities'; | ||
| import type {Image as ImageType} from '../../storefront-api-types'; | ||
| import type {PartialDeep, Simplify, SetRequired} from 'type-fest'; | ||
|
|
||
|
|
@@ -62,6 +66,10 @@ export type ShopifyImageProps = Omit<HtmlImageProps, 'src'> & { | |
| * 'src' shouldn't be passed when 'data' is used. | ||
| */ | ||
| src?: never; | ||
| /** | ||
| * An array of pixel widths to overwrite the default generated srcset. For example, `[300, 600, 800]`. | ||
| */ | ||
| widths?: (HtmlImageProps['width'] | ImageType['width'])[]; | ||
| }; | ||
|
|
||
| function ShopifyImage({ | ||
|
|
@@ -71,6 +79,7 @@ function ShopifyImage({ | |
| loading, | ||
| loader = shopifyImageLoader, | ||
| loaderOptions, | ||
| widths, | ||
| ...rest | ||
| }: ShopifyImageProps) { | ||
| if (!data.url) { | ||
|
|
@@ -107,6 +116,20 @@ function ShopifyImage({ | |
| }); | ||
| } | ||
|
|
||
| // determining what the intended width of the image is. For example, if the width is specified and lower than the image width, then that is the maximum image width | ||
| // to prevent generating a srcset with widths bigger than needed or to generate images that would distort because of being larger than original | ||
| const maxWidth = | ||
| width && finalWidth && width < finalWidth ? width : finalWidth; | ||
| const finalSrcset = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably do something similar to this for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It might be complicated for external images since we can't know how their CDN manages search parameters. We can explore this or have a curated list of CDN that we can support.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I was trying to say that on const finalSrcset = rest.srcSet ?? (loader && widths)? widths.map(width => loader(...)) : nullif that makes sense?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is now a srcset generated for ExternalImage that has a /** External images */
let finalSrcset = rest.srcSet ?? undefined;
if (!finalSrcset && loader && widths) {
// Height is a requirement in the LoaderProps, so to keep the aspect ratio, we must determine the height based on the default values
const heightToWidthRatio =
parseInt(height as string) / parseInt(width as string);
finalSrcset = widths
?.map((width) => parseInt(width as string, 10))
?.map(
(width) =>
`${loader({
...loaderOptions,
src,
width,
height: Math.floor(width * heightToWidthRatio),
})} ${width}w`
)
.join(', ');
}
/* eslint-disable hydrogen/prefer-image-component */
return (
<img
{...rest}
src={finalSrc}
width={width}
height={height}
alt={alt ?? ''}
loading={loading ?? 'lazy'}
srcSet={finalSrcset}
/>
); |
||
| rest.srcSet ?? | ||
| internalImageSrcSet({ | ||
| ...loaderOptions, | ||
| widths, | ||
| src: data.url, | ||
| width: maxWidth, | ||
| loader, | ||
| }); | ||
|
|
||
| /* eslint-disable hydrogen/prefer-image-component */ | ||
| return ( | ||
| <img | ||
|
|
@@ -117,6 +140,7 @@ function ShopifyImage({ | |
| src={finalSrc} | ||
| width={finalWidth ?? undefined} | ||
| height={finalHeight ?? undefined} | ||
| srcSet={finalSrcset} | ||
| /> | ||
| ); | ||
| /* eslint-enable hydrogen/prefer-image-component */ | ||
|
|
@@ -213,3 +237,48 @@ function ExternalImage<GenericLoaderOpts>({ | |
| ); | ||
| /* eslint-enable hydrogen/prefer-image-component */ | ||
| } | ||
|
|
||
| type InternalShopifySrcSetGeneratorsParams = Simplify< | ||
| ShopifyLoaderOptions & { | ||
| src: ImageType['url']; | ||
| widths?: (HtmlImageProps['width'] | ImageType['width'])[]; | ||
| loader?: (params: ShopifyLoaderParams) => string; | ||
| } | ||
| >; | ||
| // based on the default width sizes used by the Shopify liquid HTML tag img_tag plus a 2560 width to account for 2k resolutions | ||
| // reference: https://shopify.dev/api/liquid/filters/html-filters#image_tag | ||
| const IMG_SRC_SET_SIZES = [352, 832, 1200, 1920, 2560]; | ||
| function internalImageSrcSet({ | ||
| src, | ||
| width, | ||
| crop, | ||
| scale, | ||
| widths, | ||
| loader, | ||
| }: InternalShopifySrcSetGeneratorsParams) { | ||
| const hasCustomWidths = widths && Array.isArray(widths); | ||
| if (hasCustomWidths && widths.some((size) => isNaN(size as number))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love this check. 👍 |
||
| throw new Error( | ||
| `<Image/>: the 'widths' property of 'ShopifyLoaderOptions' must be an array of numbers` | ||
| ); | ||
|
|
||
| let setSizes = hasCustomWidths ? widths : IMG_SRC_SET_SIZES; | ||
| if ( | ||
| !hasCustomWidths && | ||
| width && | ||
| width < IMG_SRC_SET_SIZES[IMG_SRC_SET_SIZES.length - 1] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this check to make it so that there isn't a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct, It would work the same without the |
||
| ) | ||
| setSizes = IMG_SRC_SET_SIZES.filter((size) => size <= width); | ||
| const srcGenerator = loader ? loader : addImageSizeParametersToUrl; | ||
| return setSizes | ||
| .map( | ||
| (size) => | ||
| `${srcGenerator({ | ||
| src, | ||
| width: size, | ||
| crop, | ||
| scale, | ||
| })} ${size}w` | ||
| ) | ||
| .join(', '); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,6 +144,23 @@ describe('<Image />', () => { | |
| alt: 'Fancy image', | ||
| }); | ||
| }); | ||
|
|
||
| it('generates a default srcset', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you duplicate this test but instead use |
||
| const mockUrl = 'https://cdn.shopify.com/someimage.jpg'; | ||
| const sizes = [352, 832, 1200, 1920, 2560]; | ||
| const expectedSrcset = sizes | ||
| .map((size) => `${mockUrl}?width=${size} ${size}w`) | ||
| .join(', '); | ||
| const image = getPreviewImage({ | ||
| url: mockUrl, | ||
| }); | ||
|
|
||
| const component = mount(<Image data={image} />); | ||
|
|
||
| expect(component).toContainReactComponent('img', { | ||
| srcSet: expectedSrcset, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('External image', () => { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.