Skip to content

Commit

Permalink
Fix thumbnail reuse (#86)
Browse files Browse the repository at this point in the history
* Fix thumbnail reuse in PDP when no media present

Without any media present on component mount the only image will be
the thumbnail itself. Make sure the `load` event listener is only added
to the first image that is not the thumbnail.

* Remove unused onLoad prop

This didn't work with neither magnified or regular image components.
Thumbnail reuse works through the image selector ref.

* Bump version
  • Loading branch information
tonylepmets authored May 20, 2020
1 parent e6ab35f commit 6d29d73
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 7 deletions.
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-storefront",
"version": "8.3.0",
"version": "8.3.1",
"description": "Build and deploy e-commerce progressive web apps (PWAs) in record time.",
"module": "./index.js",
"license": "Apache-2.0",
Expand Down
19 changes: 14 additions & 5 deletions src/carousel/MediaCarousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import MagnifyHint from './MagnifyHint'
import CarouselThumbnails from './CarouselThumbnails'
import get from 'lodash/get'

const THUMBNAIL_IMAGE_ID = '__rsf-placeholder-thumbnail'

export const styles = theme => ({
/**
* Styles applied to the root component.
Expand Down Expand Up @@ -198,18 +200,26 @@ function MediaCarousel(props) {
})

useEffect(() => {
if (!ref.current || imagesLoaded || !thumbnail) return
const firstImage = ref.current.querySelector('img')

if (firstImage) {
if (firstImage && firstImage.id !== THUMBNAIL_IMAGE_ID) {
firstImage.addEventListener('load', onFullSizeImagesLoaded)
return () => firstImage.removeEventListener('load', onFullSizeImagesLoaded)
}
}, [])
}, [media, imagesLoaded, thumbnail])

const belowAdornments = []

if (thumbnail && !imagesLoaded) {
belowAdornments.push(<Image key="thumbnail" className={styles.thumbnail} fill {...thumbnail} />)
belowAdornments.push(
<Image
id={THUMBNAIL_IMAGE_ID}
key="thumbnail"
className={styles.thumbnail}
fill
{...thumbnail}
/>,
)
}

if (media && media.full && media.full.some(item => item.magnify)) {
Expand Down Expand Up @@ -278,7 +288,6 @@ function MediaCarousel(props) {
media.full.map((item, i) => (
<Fill height="100%" key={i}>
<MediaComponent
onLoad={i === 0 ? onFullSizeImagesLoaded : null}
magnifyProps={magnifyProps}
{...item}
src={get(item, 'magnify.src', item.src)}
Expand Down
9 changes: 9 additions & 0 deletions test/carousel/MediaCarousel.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,15 @@ describe('MediaCarousel', () => {
loadMock.mockClear()
})

it('should not add img listener if the only image is the preview thumbnail', async () => {
const loadMock = jest.spyOn(HTMLImageElement.prototype, 'addEventListener')

wrapper = mount(<MediaCarousel thumbnails={false} thumbnail={{ test: 'test' }} />)

expect(loadMock).not.toBeCalled()
loadMock.mockClear()
})

it('should have timeout on magnifying carousel', async () => {
wrapper = mount(<MediaCarousel media={media} />)

Expand Down

0 comments on commit 6d29d73

Please sign in to comment.