Fixing carousel (#113)

* Fixing carousel Making the slides virtual * Fixing carousel rendering * Fixed tests * Rendering slide in fragment * Using proper mod function in carousel thumbs * Added tests for carousel tags * Update package.json Co-authored-by: Mark Brocato <[email protected]>
storefront-foundation · Jun 25, 2020 · 0e22335 · 0e22335
1 parent be1b4be
commit 0e22335
Show file tree

Hide file tree

Showing 7 changed files with 94 additions and 18 deletions.
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "react-storefront",
-  "version": "8.11.2",
+  "version": "8.11.3",
   "description": "Build and deploy e-commerce progressive web apps (PWAs) in record time.",
   "module": "./index.js",
   "license": "Apache-2.0",

diff --git a/src/carousel/Carousel.js b/src/carousel/Carousel.js
@@ -1,11 +1,12 @@
-import React, { useState } from 'react'
+import React, { Fragment, useState } from 'react'
 import clsx from 'clsx'
 import { makeStyles } from '@material-ui/core/styles'
 import SwipeableViews from 'react-swipeable-views'
-import { autoPlay } from 'react-swipeable-views-utils'
+import { autoPlay, virtualize } from 'react-swipeable-views-utils'
 import PropTypes from 'prop-types'
 import CarouselDots from './CarouselDots'
 import CarouselArrows from './CarouselArrows'
+import mod from '../utils/mod'
 import Fill from '../Fill'
 
 const styles = theme => ({
@@ -49,7 +50,10 @@ const styles = theme => ({
 })
 
 const useStyles = makeStyles(styles, { name: 'RSFCarousel' })
-const AutoPlaySwipeableViews = autoPlay(SwipeableViews)
+
+export const AutoPlaySwipeableViews = autoPlay(SwipeableViews)
+export const VirtualizeSwipeableViews = virtualize(SwipeableViews)
+export const AutoPlayVirtualizeSwipeableViews = autoPlay(VirtualizeSwipeableViews)
 
 function useSelected(props) {
   if (props.setSelected) {
@@ -84,13 +88,26 @@ const Carousel = React.forwardRef((props, ref) => {
     indicators,
     autoplay,
     interval,
+    infinite,
   } = props
 
   classes = useStyles({ classes })
 
   const { selected, setSelected } = useSelected(props)
   const count = children && children.length
 
+  let Tag = infinite ? VirtualizeSwipeableViews : SwipeableViews
+  Tag = autoplay ? AutoPlaySwipeableViews : Tag
+  Tag = infinite && autoplay ? AutoPlayVirtualizeSwipeableViews : Tag
+
+  const slideRenderer = ({ index }) => {
+    const key = `slide-renderer-${index}`
+    const child = children[mod(index, count)]
+    if (!child) return null
+    const slide = React.cloneElement(child)
+    return <Fragment key={key}>{slide}</Fragment>
+  }
+
   return (
     <div
       ref={ref}
@@ -103,24 +120,23 @@ const Carousel = React.forwardRef((props, ref) => {
       {aboveAdornments}
       <Fill height={height}>
         <div className={classes.swipeWrap}>
-          <AutoPlaySwipeableViews
+          <Tag
             index={selected}
-            onChangeIndex={i => setSelected(i)}
+            onChangeIndex={setSelected}
             className={classes.autoPlaySwipeableViews}
             style={swipeStyle}
             slideStyle={slideStyle}
-            autoplay={autoplay}
+            slideRenderer={props.slideRenderer || slideRenderer}
             interval={interval}
             containerStyle={{ alignItems: 'center' }}
-          >
-            {children}
-          </AutoPlaySwipeableViews>
+          />
           {arrows !== false && (
             <CarouselArrows
               className={arrows === 'desktop' ? classes.hideTouchArrows : null}
               selected={selected}
               setSelected={setSelected}
               count={count}
+              infinite={infinite}
             />
           )}
           {indicators && <CarouselDots count={count} selected={selected} />}
@@ -158,6 +174,11 @@ Carousel.propTypes = {
    */
   autoplay: PropTypes.bool,
 
+  /**
+   * If true, scrolling past the last slide will cycle back to the first
+   */
+  infinite: PropTypes.bool,
+
   /**
    * The interval time (in milliseconds) for [`autoplay`](#prop-autoplay).
    */
@@ -169,6 +190,7 @@ Carousel.defaultProps = {
   arrows: 'desktop',
   autoplay: false,
   interval: 3000,
+  infinite: true,
 }
 
 export default Carousel
diff --git a/src/carousel/CarouselArrows.js b/src/carousel/CarouselArrows.js
@@ -46,7 +46,14 @@ const useStyles = makeStyles(styles, { name: 'RSFCarouselArrows' })
  * Arrows that are overlaid onto a [`Carousel`](/apiReference/carousel/Carousel) that will change
  * the slide shown when clicked.
  */
-export default function CarouselArrows({ className, classes, selected, count, setSelected }) {
+export default function CarouselArrows({
+  className,
+  classes,
+  selected,
+  count,
+  setSelected,
+  infinite,
+}) {
   classes = useStyles({ classes })
 
   const createOnClickArrow = useCallback(
@@ -59,15 +66,15 @@ export default function CarouselArrows({ className, classes, selected, count, se
 
   return (
     <div className={clsx(classes.arrows, className)}>
-      {selected !== 0 && (
+      {(selected !== 0 || infinite) && (
         <IconButton
           className={clsx(classes.arrow, classes.leftArrow)}
           onClick={createOnClickArrow(-1)}
         >
           <ChevronLeft classes={{ root: classes.icon }} />
         </IconButton>
       )}
-      {selected !== count - 1 && (
+      {(selected !== count - 1 || infinite) && (
         <IconButton
           className={clsx(classes.arrow, classes.rightArrow)}
           onClick={createOnClickArrow(1)}

diff --git a/src/carousel/CarouselThumbnails.js b/src/carousel/CarouselThumbnails.js
@@ -6,6 +6,7 @@ import Tab from '@material-ui/core/Tab'
 import { makeStyles, useTheme } from '@material-ui/core/styles'
 import useMediaQuery from '@material-ui/core/useMediaQuery'
 import Image from '../Image'
+import mod from '../utils/mod'
 
 export const styles = theme => ({
   /**
@@ -133,11 +134,12 @@ function CarouselThumbnails({
   const theme = useTheme()
   const isSmall = useMediaQuery(theme.breakpoints.down('xs'))
   const isVertical = !isSmall && ['left', 'right'].includes(thumbnailPosition)
+  const count = thumbnails.length
 
   return (
     <div className={clsx(className, styles.thumbs)}>
       <Tabs
-        value={selected}
+        value={mod(selected, count)}
         variant="scrollable"
         onChange={(_, index) => setSelected(index)}
         orientation={isVertical ? 'vertical' : 'horizontal'}

diff --git a/src/carousel/MediaCarousel.js b/src/carousel/MediaCarousel.js
@@ -164,7 +164,7 @@ function MediaCarousel(props) {
   }, [media])
 
   useEffect(() => {
-    if (media && media.full && media.full[selected].type === 'video') {
+    if (media && media.full && media.full[selected] && media.full[selected].type === 'video') {
       setVideo(true)
     } else {
       setVideo(false)

diff --git a/src/utils/mod.js b/src/utils/mod.js
@@ -0,0 +1,5 @@
+// Helper modulus function used in carousel logic
+export default function mod(index, count) {
+  const q = index % count
+  return q < 0 ? q + count : q
+}
diff --git a/test/carousel/Carousel.test.js b/test/carousel/Carousel.test.js
@@ -1,9 +1,13 @@
 import React from 'react'
 import { mount } from 'enzyme'
-import Carousel from 'react-storefront/carousel/Carousel'
+import SwipeableViews from 'react-swipeable-views'
+import Carousel, {
+  AutoPlaySwipeableViews,
+  VirtualizeSwipeableViews,
+  AutoPlayVirtualizeSwipeableViews,
+} from 'react-storefront/carousel/Carousel'
 import CarouselDots from 'react-storefront/carousel/CarouselDots'
 import CarouselArrows from 'react-storefront/carousel/CarouselArrows'
-import SwipeableViews from 'react-swipeable-views'
 
 describe('Carousel', () => {
   let wrapper
@@ -79,10 +83,46 @@ describe('Carousel', () => {
     expect(wrapper.find('#first')).toExist()
     expect(wrapper.find('#second')).not.toExist()
 
-    wrapper.find(SwipeableViews).invoke('onChangeIndex')(1)
+    wrapper.find(VirtualizeSwipeableViews).invoke('onChangeIndex')(1)
 
     expect(wrapper.find(CarouselArrows).prop('selected')).toBe(1)
     expect(wrapper.find('#first')).not.toExist()
     expect(wrapper.find('#second')).toExist()
   })
+
+  it('should use SwipeableViews when not infinite nor autoplay', () => {
+    wrapper = mount(
+      <Carousel infinite={false}>
+        <div id="foo">bar</div>
+      </Carousel>,
+    )
+    expect(wrapper.find(SwipeableViews)).toExist()
+  })
+
+  it('should use VirtualizeSwipeableViews when infinite and not autoplay', () => {
+    wrapper = mount(
+      <Carousel>
+        <div id="foo">bar</div>
+      </Carousel>,
+    )
+    expect(wrapper.find(VirtualizeSwipeableViews)).toExist()
+  })
+
+  it('should use AutoPlaySwipeableViews when not infinite and autoplay', () => {
+    wrapper = mount(
+      <Carousel infinite={false} autoplay>
+        <div id="foo">bar</div>
+      </Carousel>,
+    )
+    expect(wrapper.find(AutoPlaySwipeableViews)).toExist()
+  })
+
+  it('should use AutoPlayVirtualizeSwipeableViews when infinite and autoplay', () => {
+    wrapper = mount(
+      <Carousel autoplay>
+        <div id="foo">bar</div>
+      </Carousel>,
+    )
+    expect(wrapper.find(AutoPlayVirtualizeSwipeableViews)).toExist()
+  })
 })