Merge pull request #802 from lumapps/chore/play-stability

chore(slideshow): temporarily remove delayed visibility
lumapps · Mar 17, 2022 · f3ac5a9 · f3ac5a9
2 parents 811454f + 59d0dd2
commit f3ac5a9
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+-   Slideshow: temporarily remove slide `visibility: hidden`.
+
 ## [2.2.10][] - 2022-03-16
 
 ### Added

diff --git a/packages/lumx-react/src/components/slideshow/Slides.tsx b/packages/lumx-react/src/components/slideshow/Slides.tsx
@@ -26,11 +26,7 @@ export interface SlidesProps extends GenericProps {
     /** id to be passed in into the slides */
     slidesId?: string;
     /** callback to change whether the slideshow is playing or not */
-    setIsAutoPlaying: (isAutoPlaying: boolean) => void;
-    /** starting visible index */
-    startIndexVisible: number;
-    /** ending visible index */
-    endIndexVisible: number;
+    toggleAutoPlay: () => void;
     /** component to be rendered after the slides */
     afterSlides?: React.ReactNode;
 }
@@ -61,11 +57,8 @@ export const Slides: Comp<SlidesProps, HTMLDivElement> = forwardRef((props, ref)
         fillHeight,
         groupBy,
         isAutoPlaying,
-        autoPlay,
+        toggleAutoPlay,
         slidesId,
-        setIsAutoPlaying,
-        startIndexVisible,
-        endIndexVisible,
         children,
         afterSlides,
         ...forwardedProps
@@ -87,22 +80,12 @@ export const Slides: Comp<SlidesProps, HTMLDivElement> = forwardRef((props, ref)
             <div
                 id={slidesId}
                 className={`${CLASSNAME}__slides`}
-                onMouseEnter={() => setIsAutoPlaying(false)}
-                onMouseLeave={() => setIsAutoPlaying(Boolean(autoPlay))}
+                onMouseEnter={toggleAutoPlay}
+                onMouseLeave={toggleAutoPlay}
                 aria-live={isAutoPlaying ? 'off' : 'polite'}
             >
                 <div className={`${CLASSNAME}__wrapper`} style={wrapperStyle}>
-                    {React.Children.map(children, (child: React.ReactNode, index: number) => {
-                        if (React.isValidElement(child)) {
-                            const isCurrentlyVisible = index >= startIndexVisible && index < endIndexVisible;
-
-                            return React.cloneElement(child, {
-                                isCurrentlyVisible,
-                            });
-                        }
-
-                        return null;
-                    })}
+                    {children}
                 </div>
             </div>
 

diff --git a/packages/lumx-react/src/components/slideshow/Slideshow.tsx b/packages/lumx-react/src/components/slideshow/Slideshow.tsx
@@ -72,18 +72,15 @@ export const Slideshow: Comp<SlideshowProps, HTMLDivElement> = forwardRef((props
         setSlideshow,
         isAutoPlaying,
         slideshowSlidesId,
-        setIsAutoPlaying,
-        startIndexVisible,
-        endIndexVisible,
         slidesCount,
         onNextClick,
         onPaginationClick,
         onPreviousClick,
         slideshow,
         stopAutoPlay,
         startAutoPlay,
-        isForcePaused,
-        setIsForcePaused,
+        toggleAutoPlay,
+        toggleForcePause,
     } = SlideshowControls.useSlideshowControls({
         activeIndex,
         defaultActiveIndex: DEFAULT_PROPS.activeIndex as number,
@@ -113,9 +110,7 @@ export const Slideshow: Comp<SlideshowProps, HTMLDivElement> = forwardRef((props
             isAutoPlaying={isAutoPlaying}
             autoPlay={autoPlay}
             slidesId={slideshowSlidesId}
-            setIsAutoPlaying={setIsAutoPlaying}
-            startIndexVisible={startIndexVisible}
-            endIndexVisible={endIndexVisible}
+            toggleAutoPlay={toggleAutoPlay}
             interval={interval}
             ref={mergeRefs(ref, setSlideshow)}
             afterSlides={
@@ -143,7 +138,7 @@ export const Slideshow: Comp<SlideshowProps, HTMLDivElement> = forwardRef((props
                                 autoPlay
                                     ? {
                                           'aria-controls': slideshowSlidesId,
-                                          onClick: () => setIsForcePaused(!isForcePaused),
+                                          onClick: toggleForcePause,
                                           ...slideshowControlsProps.playButtonProps,
                                       }
                                     : undefined

diff --git a/packages/lumx-react/src/components/slideshow/SlideshowControls.stories.tsx b/packages/lumx-react/src/components/slideshow/SlideshowControls.stories.tsx
@@ -39,18 +39,15 @@ export const ControllingSlideshow = ({ theme }: any) => {
         setSlideshow,
         isAutoPlaying,
         slideshowSlidesId,
-        setIsAutoPlaying,
-        startIndexVisible,
-        endIndexVisible,
         slidesCount,
         onNextClick,
         onPaginationClick,
         onPreviousClick,
         slideshow,
-        isForcePaused,
-        setIsForcePaused,
         stopAutoPlay,
         startAutoPlay,
+        toggleAutoPlay,
+        toggleForcePause,
     } = SlideshowControls.useSlideshowControls({
         activeIndex: 0,
         defaultActiveIndex: 0,
@@ -73,11 +70,8 @@ export const ControllingSlideshow = ({ theme }: any) => {
             ref={setSlideshow}
             theme={theme}
             isAutoPlaying={isAutoPlaying}
-            autoPlay
             slidesId={slideshowSlidesId}
-            setIsAutoPlaying={setIsAutoPlaying}
-            startIndexVisible={startIndexVisible}
-            endIndexVisible={endIndexVisible}
+            toggleAutoPlay={toggleAutoPlay}
             groupBy={1}
             style={{ width: '50%' }}
             afterSlides={
@@ -96,7 +90,7 @@ export const ControllingSlideshow = ({ theme }: any) => {
                         playButtonProps={{
                             label: 'Play/Pause',
                             'aria-controls': slideshowSlidesId,
-                            onClick: () => setIsForcePaused(!isForcePaused),
+                            onClick: toggleForcePause,
                         }}
                         paginationItemLabel={(index) => `Slide ${index}`}
                     />

diff --git a/packages/lumx-react/src/components/slideshow/SlideshowItem.tsx b/packages/lumx-react/src/components/slideshow/SlideshowItem.tsx
@@ -1,13 +1,9 @@
-import React, { forwardRef, useState } from 'react';
+import React, { forwardRef } from 'react';
 
 import classNames from 'classnames';
 
 import { Comp, GenericProps, getRootClassName, handleBasicClasses } from '@lumx/react/utils';
 
-import { useDelayedVisibility } from '@lumx/react/hooks/useDelayedVisibility';
-
-import { SLIDESHOW_TRANSITION_DURATION } from '@lumx/core/js/constants';
-
 /**
  * Defines the props of the component.
  */
@@ -36,12 +32,7 @@ const CLASSNAME = getRootClassName(COMPONENT_NAME);
  * @return React element.
  */
 export const SlideshowItem: Comp<SlideshowItemProps, HTMLDivElement> = forwardRef((props, ref) => {
-    const { className, children, isCurrentlyVisible = false, ...forwardedProps } = props;
-    const [isVisible, setIsVisible] = useState<boolean>(isCurrentlyVisible);
-
-    useDelayedVisibility(isCurrentlyVisible, SLIDESHOW_TRANSITION_DURATION, (isNowVisible) => {
-        setIsVisible(isNowVisible);
-    });
+    const { className, children, ...forwardedProps } = props;
 
     return (
         <div
@@ -55,8 +46,6 @@ export const SlideshowItem: Comp<SlideshowItemProps, HTMLDivElement> = forwardRe
             aria-roledescription="slide"
             role="group"
             {...forwardedProps}
-            style={!isVisible ? { visibility: 'hidden', ...(forwardedProps.style || {}) } : forwardedProps.style || {}}
-            aria-hidden={!isVisible}
         >
             {children}
         </div>

diff --git a/packages/lumx-react/src/components/slideshow/__snapshots__/Slideshow.test.tsx.snap b/packages/lumx-react/src/components/slideshow/__snapshots__/Slideshow.test.tsx.snap
@@ -33,20 +33,18 @@ Array [
       </div>
     }
     autoPlay={false}
-    endIndexVisible={1}
     groupBy={1}
     id="slideshow1"
     interval={1000}
     isAutoPlaying={false}
-    setIsAutoPlaying={[Function]}
     slidesId="slideshow-slides2"
-    startIndexVisible={0}
     style={
       Object {
         "width": "50%",
       }
     }
     theme="light"
+    toggleAutoPlay={[Function]}
   >
     <SlideshowItem
       key="/demo-assets/landscape1.jpg-0"

diff --git a/packages/lumx-react/src/hooks/useSlideshowControls.ts b/packages/lumx-react/src/hooks/useSlideshowControls.ts
@@ -52,10 +52,10 @@ export interface UseSlideshowControls {
     isAutoPlaying: boolean;
     /** whether the slideshow was force paused or not */
     isForcePaused: boolean;
-    /** callback to enable/disable the force pause feature */
-    setIsForcePaused: (isForcePaused: boolean) => void;
     /** callback to change whether the slideshow is autoplaying or not */
-    setIsAutoPlaying: (isAutoPlaying: boolean) => void;
+    toggleAutoPlay: () => void;
+    /** calback to change whether the slideshow should be force paused or not */
+    toggleForcePause: () => void;
     /** current active slide index  */
     activeIndex: number;
     /** set the current index as the active one */
@@ -141,10 +141,19 @@ export const useSlideshowControls = ({
         }
     }, [currentIndex, slidesCount, defaultActiveIndex]);
 
+    const startAutoPlay = () => {
+        setIsAutoPlaying(Boolean(autoPlay));
+    };
+
+    const stopAutoPlay = () => {
+        setIsAutoPlaying(false);
+    };
+
     // Handle click on a bullet to go to a specific slide.
     const onPaginationClick = useCallback(
         (index: number) => {
-            setIsAutoPlaying(false);
+            stopAutoPlay();
+            setIsForcePaused(true);
 
             if (index >= 0 && index < slidesCount) {
                 setCurrentIndex(index);
@@ -156,7 +165,8 @@ export const useSlideshowControls = ({
     // Handle click or keyboard event to go to next slide.
     const onNextClick = useCallback(
         (loopback = true) => {
-            setIsAutoPlaying(false);
+            stopAutoPlay();
+            setIsForcePaused(true);
             goToNextSlide(loopback);
         },
         [goToNextSlide],
@@ -165,7 +175,8 @@ export const useSlideshowControls = ({
     // Handle click or keyboard event to go to previous slide.
     const onPreviousClick = useCallback(
         (loopback = true) => {
-            setIsAutoPlaying(false);
+            stopAutoPlay();
+            setIsForcePaused(true);
             goToPreviousSlide(loopback);
         },
         [goToPreviousSlide],
@@ -185,21 +196,23 @@ export const useSlideshowControls = ({
     const slideshowId = useMemo(() => id || uniqueId('slideshow'), [id]);
     const slideshowSlidesId = useMemo(() => slidesId || uniqueId('slideshow-slides'), [slidesId]);
 
-    const startAutoPlay = () => {
-        setIsAutoPlaying(Boolean(autoPlay));
+    const toggleAutoPlay = () => {
+        if (isSlideshowAutoPlaying) {
+            stopAutoPlay();
+        } else {
+            startAutoPlay();
+        }
     };
 
-    const stopAutoPlay = () => {
-        setIsAutoPlaying(false);
-    };
+    const toggleForcePause = () => {
+        const shouldBePaused = !isForcePaused;
 
-    const forcePause = (isPaused: boolean) => {
-        setIsForcePaused(isPaused);
+        setIsForcePaused(shouldBePaused);
 
-        if (isPaused) {
-            stopAutoPlay();
-        } else {
+        if (!shouldBePaused) {
             startAutoPlay();
+        } else {
+            stopAutoPlay();
         }
     };
 
@@ -218,13 +231,13 @@ export const useSlideshowControls = ({
         onNextClick,
         onPaginationClick,
         isAutoPlaying: isSlideshowAutoPlaying,
-        setIsAutoPlaying,
+        toggleAutoPlay,
         activeIndex: currentIndex,
         slidesCount,
         setActiveIndex: setCurrentIndex,
         startAutoPlay,
         stopAutoPlay,
         isForcePaused,
-        setIsForcePaused: forcePause,
+        toggleForcePause,
     };
 };