fix: streamline slide keys usage (fix #265)

igordanchenko · Apr 25, 2024 · 331e9cb · 331e9cb
1 parent e4e0d4e
commit 331e9cb
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 85 deletions.
diff --git a/src/modules/Carousel.tsx b/src/modules/Carousel.tsx
@@ -8,6 +8,8 @@ import {
   composePrefix,
   cssClass,
   cssVar,
+  getSlide,
+  getSlideKey,
   hasSlides,
   isImageSlide,
   parseLengthPercentage,
@@ -114,35 +116,23 @@ export function Carousel({ carousel }: ComponentProps) {
   const spacingValue = parseLengthPercentage(carousel.spacing);
   const paddingValue = parseLengthPercentage(carousel.padding);
 
-  const items = [];
   const preload = calculatePreload(carousel, slides, 1);
+  const items: ({ key: React.Key } & ({ slide: Slide; offset: number } | { slide?: never; offset?: never }))[] = [];
 
   if (hasSlides(slides)) {
-    for (let i = currentIndex - preload; i < currentIndex; i += 1) {
-      const key = globalIndex + i - currentIndex;
-      items.push(
-        !carousel.finite || i >= 0 ? (
-          <CarouselSlide
-            key={key}
-            slide={slides[(i + preload * slides.length) % slides.length]}
-            offset={i - currentIndex}
-          />
-        ) : (
-          <Placeholder key={key} />
-        ),
-      );
-    }
+    for (let index = currentIndex - preload; index <= currentIndex + preload; index += 1) {
+      const slide = getSlide(slides, index);
+      const key = globalIndex - currentIndex + index;
+      const placeholder = carousel.finite && (index < 0 || index > slides.length - 1);
 
-    items.push(<CarouselSlide key={globalIndex} slide={slides[currentIndex]} offset={0} />);
-
-    for (let i = currentIndex + 1; i <= currentIndex + preload; i += 1) {
-      const key = globalIndex + i - currentIndex;
       items.push(
-        !carousel.finite || i <= slides.length - 1 ? (
-          <CarouselSlide key={key} slide={slides[i % slides.length]} offset={i - currentIndex} />
-        ) : (
-          <Placeholder key={key} />
-        ),
+        !placeholder
+          ? {
+              key: [`${key}`, getSlideKey(slide)].filter(Boolean).join("|"),
+              offset: index - currentIndex,
+              slide,
+            }
+          : { key },
       );
     }
   }
@@ -159,7 +149,9 @@ export function Carousel({ carousel }: ComponentProps) {
         [`${cssVar(cssPrefix("padding_percent"))}`]: paddingValue.percent || 0,
       }}
     >
-      {items}
+      {items.map(({ key, slide, offset }) =>
+        slide ? <CarouselSlide key={key} slide={slide} offset={offset} /> : <Placeholder key={key} />,
+      )}
     </div>
   );
 }

diff --git a/src/plugins/thumbnails/Thumbnail.tsx b/src/plugins/thumbnails/Thumbnail.tsx
@@ -43,7 +43,16 @@ function renderThumbnail({ slide, render, rect, imageFit }: RenderThumbnailProps
   if (slide.type === "video") {
     return (
       <>
-        {slide.poster && <ImageSlide slide={{ src: slide.poster }} render={render} rect={rect} imageFit={imageFit} />}
+        {slide.poster && (
+          <ImageSlide
+            // video slides do not provide a unique key in the `ThumbnailsTrack`
+            key={slide.poster}
+            slide={{ src: slide.poster }}
+            render={render}
+            rect={rect}
+            imageFit={imageFit}
+          />
+        )}
 
         <VideoThumbnailIcon className={thumbnailIconClass} />
       </>

diff --git a/src/plugins/thumbnails/ThumbnailsTrack.tsx b/src/plugins/thumbnails/ThumbnailsTrack.tsx
@@ -11,6 +11,7 @@ import {
   cssClass,
   cssVar,
   getSlide,
+  getSlideKey,
   hasSlides,
   Slide,
   useAnimation,
@@ -54,7 +55,6 @@ export function ThumbnailsTrack({ visible, containerRef }: ThumbnailsTrackProps)
   const { position, width, height, border, borderStyle, borderColor, borderRadius, padding, gap, vignette } =
     thumbnails;
 
-  const index = globalIndex;
   const animationDuration = animation?.duration || 0;
   const offset = (animationDuration > 0 && animation?.increment) || 0;
 
@@ -92,46 +92,30 @@ export function ThumbnailsTrack({ visible, containerRef }: ThumbnailsTrackProps)
   React.useEffect(() => cleanup(subscribe(ACTION_SWIPE, handleControllerSwipe)), [subscribe, handleControllerSwipe]);
 
   const preload = calculatePreload(carousel, slides);
-
-  const items: { slide: Slide | null; index: number; placeholder?: boolean }[] = [];
+  const items: { key: React.Key; index: number; slide: Slide | null }[] = [];
 
   if (hasSlides(slides)) {
-    if (offset < 0) {
-      for (let i = index - preload + offset; i < index - preload; i += 1) {
-        items.push({ slide: null, index: i, placeholder: true });
-      }
-    }
-
-    for (let i = index - preload - Math.max(offset, 0); i < index; i += 1) {
-      if (!(carousel.finite && i < 0)) {
-        items.push({ slide: getSlide(slides, i), index: i });
-      } else {
-        items.push({ slide: null, index: i, placeholder: true });
-      }
-    }
-
-    items.push({ slide: getSlide(slides, index), index });
-
-    for (let i = index + 1; i <= index + preload - Math.min(offset, 0); i += 1) {
-      if (!carousel.finite || i <= slides.length - 1) {
-        items.push({ slide: getSlide(slides, i), index: i });
-      } else {
-        items.push({ slide: null, index: i, placeholder: true });
-      }
-    }
-
-    if (offset > 0) {
-      for (let i = index + preload + 1; i <= index + preload + offset; i += 1) {
-        items.push({ slide: null, index: i, placeholder: true });
-      }
+    for (
+      let index = globalIndex - preload - Math.abs(offset);
+      index <= globalIndex + preload + Math.abs(offset);
+      index += 1
+    ) {
+      const placeholder =
+        (carousel.finite && (index < 0 || index > slides.length - 1)) ||
+        (offset < 0 && index < globalIndex - preload) ||
+        (offset > 0 && index > globalIndex + preload);
+      const slide = !placeholder ? getSlide(slides, index) : null;
+      const key = [`${index}`, slide ? getSlideKey(slide) : "placeholder"].filter(Boolean).join("|");
+
+      items.push({ key, index, slide });
     }
   }
 
   const handleClick = (slideIndex: number) => () => {
-    if (slideIndex > index) {
-      publish(ACTION_NEXT, { count: slideIndex - index });
-    } else if (slideIndex < index) {
-      publish(ACTION_PREV, { count: index - slideIndex });
+    if (slideIndex > globalIndex) {
+      publish(ACTION_NEXT, { count: slideIndex - globalIndex });
+    } else if (slideIndex < globalIndex) {
+      publish(ACTION_PREV, { count: globalIndex - slideIndex });
     }
   };
 
@@ -166,41 +150,41 @@ export function ThumbnailsTrack({ visible, containerRef }: ThumbnailsTrackProps)
         tabIndex={-1}
         {...registerSensors}
       >
-        {items.map(({ slide, index: slideIndex, placeholder }) => {
+        {items.map(({ key, index, slide }) => {
           const fadeAnimationDuration = animationDuration / Math.abs(offset || 1);
 
           const fadeIn =
-            (offset > 0 && slideIndex > index + preload - offset && slideIndex <= index + preload) ||
-            (offset < 0 && slideIndex < index - preload - offset && slideIndex >= index - preload)
+            (offset > 0 && index > globalIndex + preload - offset && index <= globalIndex + preload) ||
+            (offset < 0 && index < globalIndex - preload - offset && index >= globalIndex - preload)
               ? {
                   duration: fadeAnimationDuration,
                   delay:
-                    ((offset > 0 ? slideIndex - (index + preload - offset) : index - preload - offset - slideIndex) -
+                    ((offset > 0 ? index - (globalIndex + preload - offset) : globalIndex - preload - offset - index) -
                       1) *
                     fadeAnimationDuration,
                 }
               : undefined;
 
           const fadeOut =
-            (offset > 0 && slideIndex < index - preload) || (offset < 0 && slideIndex > index + preload)
+            (offset > 0 && index < globalIndex - preload) || (offset < 0 && index > globalIndex + preload)
               ? {
                   duration: fadeAnimationDuration,
                   delay:
                     (offset > 0
-                      ? offset - (index - preload - slideIndex)
-                      : -offset - (slideIndex - (index + preload))) * fadeAnimationDuration,
+                      ? offset - (globalIndex - preload - index)
+                      : -offset - (index - (globalIndex + preload))) * fadeAnimationDuration,
                 }
               : undefined;
 
           return (
             <Thumbnail
-              key={[`${slideIndex}`, placeholder && "placeholder"].filter(Boolean).join("-")}
+              key={key}
               slide={slide}
-              active={slideIndex === index}
+              active={index === globalIndex}
               fadeIn={fadeIn}
               fadeOut={fadeOut}
-              placeholder={Boolean(placeholder)}
-              onClick={handleClick(slideIndex)}
+              placeholder={!slide}
+              onClick={handleClick(index)}
               onLoseFocus={() => track.current?.focus()}
             />
           );

diff --git a/src/plugins/video/Video.tsx b/src/plugins/video/Video.tsx
@@ -12,14 +12,12 @@ function isVideoSlide(slide: Slide): slide is SlideVideo {
 export function Video({ augment }: PluginProps) {
   augment(({ render: { slide: renderSlide, ...restRender }, video, ...restProps }) => ({
     render: {
-      slide: ({ slide, offset, rect }) => {
-        if (isVideoSlide(slide)) {
-          return (
-            <VideoSlide key={slide.sources?.map((source) => source.src).join(" ")} slide={slide} offset={offset} />
-          );
-        }
-        return renderSlide?.({ slide, offset, rect });
-      },
+      slide: ({ slide, offset, rect }) =>
+        isVideoSlide(slide) ? (
+          <VideoSlide key={slide.sources?.map((source) => source.src).join("|")} slide={slide} offset={offset} />
+        ) : (
+          renderSlide?.({ slide, offset, rect })
+        ),
       ...restRender,
     },
     video: resolveVideoProps(video),

diff --git a/src/plugins/video/VideoSlide.tsx b/src/plugins/video/VideoSlide.tsx
@@ -133,9 +133,8 @@ export function VideoSlide({ slide, offset }: VideoSlideProps) {
                 publish(ACTIVE_SLIDE_COMPLETE);
               }}
             >
-              {sources.map(({ src, type }, index) => (
-                // eslint-disable-next-line react/no-array-index-key
-                <source key={index} src={src} type={type} />
+              {sources.map(({ src, type }) => (
+                <source key={[src, type].join("|")} src={src} type={type} />
               ))}
             </video>
           )}

diff --git a/src/plugins/zoom/ZoomWrapper.tsx b/src/plugins/zoom/ZoomWrapper.tsx
@@ -54,14 +54,12 @@ export function ZoomWrapper({ render, slide, offset, rect }: ZoomWrapperProps) {
 
     rendered = isResponsiveImageSlide(slide) ? (
       <ResponsiveImage
-        key={slide.src}
         {...slideProps}
         slide={slide}
         rect={offset === 0 ? { width: rect.width * zoom, height: rect.height * zoom } : rect}
       />
     ) : (
       <ImageSlide
-        key={slide.src}
         onLoad={(img) => setImageDimensions({ width: img.naturalWidth, height: img.naturalHeight })}
         {...slideProps}
       />

diff --git a/src/utils.ts b/src/utils.ts
@@ -120,6 +120,11 @@ export function getSlideIfPresent(slides: Slide[], index: number) {
   return hasSlides(slides) ? getSlide(slides, index) : undefined;
 }
 
+export function getSlideKey(slide: Slide) {
+  // TODO v4: add `key` attribute to GenericSlide
+  return isImageSlide(slide) ? slide.src : undefined;
+}
+
 export function addToolbarButton(toolbar: ToolbarSettings, key: string, button: React.ReactNode) {
   if (!button) return toolbar;