feat: use `<picture>` elements for responsive images @W-18714493 by seckardt · Pull Request #2724 · SalesforceCommerceCloud/pwa-kit

seckardt · 2025-07-03T09:00:39Z

Description

This PR introduces using <picture> elements as the base for responsive images created by the <DynamicImage/> component. This change is needed to support art direction of images on our pages, but most importantly paves the way for using modern image formats like AVIF or WebP in our Dynamic Imaging Service (DIS), which is targeted for the next minor release already. The change also adds responsive preloading to the mix, which is otherwise tricky to get right.

This PR additionally adjusts the sizes of the responsive images on the PLP, because previously defined dimensions (see PR #2446) forgot to take the existence and the impact of the search refinements panel into account, which lead to a significant over-fetching of images (~400-500kB per page on e.g. a larger Mac desktop).

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Changes

(change1)

How to Test-Drive This PR

(step1)

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

cc-prodsec · 2025-07-03T09:00:49Z

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

packages/template-retail-react-app/app/utils/responsive-image.js

joeluong-sfcc

Can you add details/demo on how to test this?

packages/template-retail-react-app/app/utils/responsive-image.js

packages/template-retail-react-app/app/components/dynamic-image/index.jsx

packages/template-retail-react-app/app/utils/responsive-image.js

Irrelevant reasons. Also misses the point that we simply have to make that change to only to be able support modern image formats in one of the next two sprints.

…es @W-18960755

bfeister · 2025-07-14T20:05:31Z

packages/template-retail-react-app/app/components/dynamic-image/index.jsx

-        () => getResponsiveImageAttributes({src, widths, breakpoints: theme.breakpoints}),
-        [src, widths, theme.breakpoints]
-    )
+    const [responsiveImageProps, numSources, effectiveImageProps, responsiveLinks] = useMemo(() => {


Glad to see this is memoized

bfeister · 2025-07-14T20:08:45Z

packages/template-retail-react-app/app/utils/responsive-image.js

+const vwValue = /^\d+vw$/
+const pxValue = /^\d+px$/
+const emValue = /^\d+em$/


For a moment I was worried as I traversed a few function call depths that we weren't catching / throwing for values outside the pattern. Looks good 👍

bfeister · 2025-07-14T20:10:58Z

packages/template-retail-react-app/app/components/dynamic-image/index.test.js

+            expect(elements[0]).toHaveAttribute('decoding', 'async')
+            expect(wrapper.firstElementChild).toBe(elements[0])
+        })
+
+        test('renders an image with explicit widths', () => {
+            const {getByTestId, getAllByTitle} = renderWithProviders(
+                <DynamicImage
+                    data-testid={'dynamic-image'}
+                    src={src}
+                    imageProps={{
+                        ...imageProps,
+                        loading: 'lazy'
+                    }}
+                    widths={['50vw', '50vw', '20vw', '20vw', '25vw']}
+                />
+            )
+
+            const wrapper = getByTestId('dynamic-image')
+            const elements = getAllByTitle(imageProps.title)
+            expect(elements).toHaveLength(1)
+            expect(elements[0]).toHaveAttribute('src', src)
+            expect(elements[0]).toHaveAttribute('loading', 'lazy')
+            expect(elements[0]).toHaveAttribute('decoding', 'async')
+            expect(elements[0]).not.toHaveAttribute('sizes')
+            expect(elements[0]).not.toHaveAttribute('srcset')
+
+            expect(wrapper.firstElementChild).not.toBe(elements[0])
+            expect(wrapper.firstElementChild.tagName.toLowerCase()).toBe('picture')
+
+            const sourceElements = Array.from(wrapper.querySelectorAll('source'))
+            expect(sourceElements).toHaveLength(5)
+            expect(sourceElements[0]).toHaveAttribute('media', '(min-width: 80em)')
+            expect(sourceElements[0]).toHaveAttribute('sizes', '25vw')
+            expect(sourceElements[0]).toHaveAttribute(
+                'srcset',
+                [384, 768].map((width) => `${src} ${width}w`).join(', ')
+            )
+            expect(sourceElements[1]).toHaveAttribute('media', '(min-width: 62em)')
+            expect(sourceElements[1]).toHaveAttribute('sizes', '20vw')
+            expect(sourceElements[1]).toHaveAttribute(
+                'srcset',
+                [256, 512].map((width) => `${src} ${width}w`).join(', ')
+            )
+            expect(sourceElements[2]).toHaveAttribute('media', '(min-width: 48em)')
+            expect(sourceElements[2]).toHaveAttribute('sizes', '20vw')
+            expect(sourceElements[2]).toHaveAttribute(
+                'srcset',
+                [198, 396].map((width) => `${src} ${width}w`).join(', ')
+            )
+            expect(sourceElements[3]).toHaveAttribute('media', '(min-width: 30em)')
+            expect(sourceElements[3]).toHaveAttribute('sizes', '50vw')
+            expect(sourceElements[3]).toHaveAttribute(
+                'srcset',
+                [384, 768].map((width) => `${src} ${width}w`).join(', ')
+            )
+            expect(sourceElements[4]).not.toHaveAttribute('media')
+            expect(sourceElements[4]).toHaveAttribute('sizes', '50vw')
+            expect(sourceElements[4]).toHaveAttribute(
+                'srcset',
+                [240, 480].map((width) => `${src} ${width}w`).join(', ')
+            )
+
+            expect(Helmet.peek()?.linkTags ?? []).toStrictEqual([])


Not seeing an explicit test for the error thrown by invalid component usage, e.g.

<DynamicImage data-testid={'dynamic-image'} src={src} imageProps={{ ...imageProps, loading: 'lazy' }} widths={['INVALID']} />

Hmm, in that case it wasn't there before either. Didn't add/change anything to the widths logic/handling.

bfeister · 2025-07-14T20:12:11Z

packages/template-retail-react-app/app/pages/product-list/index.jsx

+                                                                      // Each product image can take up the full 50% of the screen width
+                                                                      '50vw', // base <= 479px
+                                                                      '50vw', // sm >= 480px ; <= 767px


It does feel good to move away from the density logic that was here in the previous iteration 👍

I'm not quite sure yet. The unconditional addition of images for double density and thus having no influence on the behavior at all is not necessarily the real thing either.

bfeister

Looks good to me. Very interested to hear the lab improvements yielded by this

Signed-off-by: Steffen Eckardt <3235219+seckardt@users.noreply.github.com>

seckardt requested a review from a team as a code owner July 3, 2025 09:00

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch 5 times, most recently from 65eab27 to dfd8db9 Compare July 6, 2025 08:10

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

adamraya reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

joeluong-sfcc reviewed Jul 8, 2025

View reviewed changes

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch from bc7239b to 0a86480 Compare July 8, 2025 19:22

seckardt changed the title ~~fix: address LCP regression on PLP caused by responsive image densities @W-18960755~~ fix: use <picture> elements for responsive images and support image densities @W-18714493 Jul 8, 2025

alexvuong reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/components/dynamic-image/index.jsx Outdated Show resolved Hide resolved

alexvuong reviewed Jul 8, 2025

View reviewed changes

packages/template-retail-react-app/app/components/dynamic-image/index.jsx Show resolved Hide resolved

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch 4 times, most recently from 2af870d to facbc52 Compare July 9, 2025 08:36

joeluong-sfcc reviewed Jul 10, 2025

View reviewed changes

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch 3 times, most recently from 0edf32c to 5315c47 Compare July 10, 2025 17:18

alexvuong reviewed Jul 10, 2025

View reviewed changes

packages/template-retail-react-app/app/utils/responsive-image.js Outdated Show resolved Hide resolved

seckardt dismissed joeluong-sfcc’s stale review via d7a3000 July 11, 2025 06:56

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch from 5315c47 to d7a3000 Compare July 11, 2025 06:56

seckardt changed the title ~~fix: use <picture> elements for responsive images and support image densities @W-18714493~~ feat: use <picture> elements for responsive images and support image densities @W-18714493 Jul 11, 2025

seckardt added 2 commits July 11, 2025 09:17

fix: address LCP regression on PLP caused by responsive image densiti…

717940d

…es @W-18960755

feat: use <picture> element for responsive images @W-18714493

b7ccb8d

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch from d7a3000 to b7ccb8d Compare July 11, 2025 07:17

seckardt requested review from alexvuong and joeluong-sfcc July 11, 2025 07:17

joeluong-sfcc previously approved these changes Jul 11, 2025

View reviewed changes

seckardt dismissed joeluong-sfcc’s stale review via d7e8992 July 14, 2025 12:08

seckardt changed the title ~~feat: use <picture> elements for responsive images and support image densities @W-18714493~~ feat: use <picture> elements for responsive images @W-18714493 Jul 14, 2025

feat: solve image over-fetching without density adjustments @W-18714493

5051b3a

seckardt force-pushed the fix/258-W-18960755-plp-lcp-regression branch from d7e8992 to 5051b3a Compare July 14, 2025 16:38

bfeister reviewed Jul 14, 2025

View reviewed changes

bfeister previously approved these changes Jul 14, 2025

View reviewed changes

joeluong-sfcc previously approved these changes Jul 15, 2025

View reviewed changes

Merge branch 'develop' into fix/258-W-18960755-plp-lcp-regression

c08463a

Signed-off-by: Steffen Eckardt <3235219+seckardt@users.noreply.github.com>

seckardt dismissed stale reviews from joeluong-sfcc and bfeister via c08463a July 16, 2025 18:02

seckardt enabled auto-merge (squash) July 16, 2025 18:02

bfeister approved these changes Jul 16, 2025

View reviewed changes

joeluong-sfcc approved these changes Jul 16, 2025

View reviewed changes

seckardt merged commit 0c6d073 into develop Jul 16, 2025
36 checks passed

seckardt deleted the fix/258-W-18960755-plp-lcp-regression branch July 17, 2025 04:16

vcua-mobify mentioned this pull request Jul 18, 2025

@W-18955816@ PWA Kit 3.11.0 preview.1 release #2853

Merged

kumaravinashcommercecloud pushed a commit that referenced this pull request Aug 12, 2025

feat: use <picture> elements for responsive images @W-18714493 (#2724)

7d98e2b

Conversation

seckardt commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

Uh oh!

cc-prodsec commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joeluong-sfcc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfeister Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

bfeister Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

bfeister Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seckardt Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

bfeister Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

seckardt Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

bfeister left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

seckardt commented Jul 3, 2025 •

edited

Loading

cc-prodsec commented Jul 3, 2025 •

edited

Loading

bfeister Jul 14, 2025 •

edited

Loading