Fix scrollable containers by adjusting islands logic (#12673)

* Make scrollable containers islands, moving the scrollable island wrapper to decide container * Move comments explaining why the scrollable containers need to be islands
guardian · Oct 24, 2024 · b41b474 · b41b474
1 parent 5b5dee5
commit b41b474
Show file tree

Hide file tree

Showing 6 changed files with 91 additions and 95 deletions.
diff --git a/dotcom-rendering/src/components/DecideContainer.tsx b/dotcom-rendering/src/components/DecideContainer.tsx
@@ -27,8 +27,8 @@ import { FlexibleSpecial } from './FlexibleSpecial';
 import { Island } from './Island';
 import { NavList } from './NavList';
 import { ScrollableHighlights } from './ScrollableHighlights.importable';
-import { ScrollableMedium } from './ScrollableMedium';
-import { ScrollableSmall } from './ScrollableSmall';
+import { ScrollableMedium } from './ScrollableMedium.importable';
+import { ScrollableSmall } from './ScrollableSmall.importable';
 
 type Props = {
 	trails: DCRFrontCard[];
@@ -253,25 +253,29 @@ export const DecideContainer = ({
 			);
 		case 'scrollable/small':
 			return (
-				<ScrollableSmall
-					trails={trails}
-					imageLoading={imageLoading}
-					containerType={'scrollable/small'}
-					containerPalette={containerPalette}
-					showAge={showAge}
-					absoluteServerTimes={absoluteServerTimes}
-				/>
+				<Island priority="feature" defer={{ until: 'visible' }}>
+					<ScrollableSmall
+						trails={trails}
+						imageLoading={imageLoading}
+						containerType={'scrollable/small'}
+						containerPalette={containerPalette}
+						showAge={showAge}
+						absoluteServerTimes={absoluteServerTimes}
+					/>
+				</Island>
 			);
 		case 'scrollable/medium':
 			return (
-				<ScrollableMedium
-					trails={trails}
-					imageLoading={imageLoading}
-					containerType={'scrollable/small'}
-					containerPalette={containerPalette}
-					showAge={showAge}
-					absoluteServerTimes={absoluteServerTimes}
-				/>
+				<Island priority="feature" defer={{ until: 'visible' }}>
+					<ScrollableMedium
+						trails={trails}
+						imageLoading={imageLoading}
+						containerType={'scrollable/small'}
+						containerPalette={containerPalette}
+						showAge={showAge}
+						absoluteServerTimes={absoluteServerTimes}
+					/>
+				</Island>
 			);
 		case 'scrollable/feature':
 		case 'static/feature/2':

diff --git a/...ponents/ScrollableCarousel.Importable.tsx → ...ing/src/components/ScrollableCarousel.tsx b/...ponents/ScrollableCarousel.Importable.tsx → ...ing/src/components/ScrollableCarousel.tsx
@@ -153,10 +153,6 @@ const generateCarouselColumnStyles = (totalCards: number) => {
 
 /**
  * A component used in the carousel fronts containers (e.g. small/medium/feature)
- *
- *  ## Why does this need to be an Island?
- *
- * The carouselling arrow buttons need to run javascript.
  */
 export const ScrollableCarousel = ({ children, carouselLength }: Props) => {
 	const carouselRef = useRef<HTMLOListElement | null>(null);

diff --git a/...ering/src/components/ScrollableMedium.tsx → ...omponents/ScrollableMedium.importable.tsx b/...ering/src/components/ScrollableMedium.tsx → ...omponents/ScrollableMedium.importable.tsx
@@ -7,8 +7,7 @@ import type {
 	DCRFrontCard,
 } from '../types/front';
 import { FrontCard } from './FrontCard';
-import { Island } from './Island';
-import { ScrollableCarousel } from './ScrollableCarousel.Importable';
+import { ScrollableCarousel } from './ScrollableCarousel';
 
 type Props = {
 	trails: DCRFrontCard[];
@@ -52,6 +51,10 @@ const verticalLineStyles = css`
 
 /**
  * A container used on fronts to display a carousel of small cards
+ *
+ * ## Why does this need to be an Island?
+ *
+ * The carouselling arrow buttons need to run javascript.
  */
 export const ScrollableMedium = ({
 	trails,
@@ -62,40 +65,35 @@ export const ScrollableMedium = ({
 	showAge,
 }: Props) => {
 	return (
-		<Island priority="feature" defer={{ until: 'visible' }}>
-			<ScrollableCarousel carouselLength={trails.length}>
-				{trails.map((trail) => {
-					return (
-						<li
-							key={trail.url}
-							css={[itemStyles, verticalLineStyles]}
-						>
-							<FrontCard
-								trail={trail}
-								imageLoading={imageLoading}
-								absoluteServerTimes={!!absoluteServerTimes}
-								containerPalette={containerPalette}
-								containerType={containerType}
-								showAge={!!showAge}
-								headlineSizes={{
-									desktop: 'xsmall',
-									tablet: 'xxsmall',
-								}}
-								imagePositionOnDesktop="bottom"
-								imagePositionOnMobile="bottom"
-								imageSize="small" // TODO - needs fixed width images
-								trailText={undefined} // unsupported
-								supportingContent={undefined} // unsupported
-								aspectRatio="5:4"
-								kickerText={trail.kickerText}
-								showLivePlayable={trail.showLivePlayable}
-								showTopBarDesktop={false}
-								showTopBarMobile={false}
-							/>
-						</li>
-					);
-				})}
-			</ScrollableCarousel>
-		</Island>
+		<ScrollableCarousel carouselLength={trails.length}>
+			{trails.map((trail) => {
+				return (
+					<li key={trail.url} css={[itemStyles, verticalLineStyles]}>
+						<FrontCard
+							trail={trail}
+							imageLoading={imageLoading}
+							absoluteServerTimes={!!absoluteServerTimes}
+							containerPalette={containerPalette}
+							containerType={containerType}
+							showAge={!!showAge}
+							headlineSizes={{
+								desktop: 'xsmall',
+								tablet: 'xxsmall',
+							}}
+							imagePositionOnDesktop="bottom"
+							imagePositionOnMobile="bottom"
+							imageSize="small" // TODO - needs fixed width images
+							trailText={undefined} // unsupported
+							supportingContent={undefined} // unsupported
+							aspectRatio="5:4"
+							kickerText={trail.kickerText}
+							showLivePlayable={trail.showLivePlayable}
+							showTopBarDesktop={false}
+							showTopBarMobile={false}
+						/>
+					</li>
+				);
+			})}
+		</ScrollableCarousel>
 	);
 };
diff --git a/dotcom-rendering/src/components/ScrollableMedium.stories.tsx b/dotcom-rendering/src/components/ScrollableMedium.stories.tsx
@@ -2,7 +2,7 @@ import type { Meta, StoryObj } from '@storybook/react';
 import { discussionApiUrl } from '../../fixtures/manual/discussionApiUrl';
 import { trails } from '../../fixtures/manual/highlights-trails';
 import { FrontSection } from './FrontSection';
-import { ScrollableMedium } from './ScrollableMedium';
+import { ScrollableMedium } from './ScrollableMedium.importable';
 
 export default {
 	title: 'Components/ScrollableMedium',

diff --git a/...dering/src/components/ScrollableSmall.tsx → ...components/ScrollableSmall.importable.tsx b/...dering/src/components/ScrollableSmall.tsx → ...components/ScrollableSmall.importable.tsx
@@ -7,8 +7,7 @@ import type {
 	DCRFrontCard,
 } from '../types/front';
 import { FrontCard } from './FrontCard';
-import { Island } from './Island';
-import { ScrollableCarousel } from './ScrollableCarousel.Importable';
+import { ScrollableCarousel } from './ScrollableCarousel';
 
 type Props = {
 	trails: DCRFrontCard[];
@@ -52,6 +51,10 @@ const verticalLineStyles = css`
 
 /**
  * A container used on fronts to display a carousel of small cards
+ *
+ * ## Why does this need to be an Island?
+ *
+ * The carouselling arrow buttons need to run javascript.
  */
 export const ScrollableSmall = ({
 	trails,
@@ -62,37 +65,32 @@ export const ScrollableSmall = ({
 	showAge,
 }: Props) => {
 	return (
-		<Island priority="feature" defer={{ until: 'visible' }}>
-			<ScrollableCarousel carouselLength={trails.length}>
-				{trails.map((trail) => {
-					return (
-						<li
-							key={trail.url}
-							css={[itemStyles, verticalLineStyles]}
-						>
-							<FrontCard
-								trail={trail}
-								imageLoading={imageLoading}
-								absoluteServerTimes={!!absoluteServerTimes}
-								containerPalette={containerPalette}
-								containerType={containerType}
-								showAge={!!showAge}
-								headlineSizes={{ desktop: 'xxsmall' }}
-								imagePositionOnDesktop="left"
-								imagePositionOnMobile="left"
-								imageSize="small" // TODO - needs fixed width images
-								trailText={undefined} // unsupported
-								supportingContent={undefined} // unsupported
-								aspectRatio="5:4"
-								kickerText={trail.kickerText}
-								showLivePlayable={trail.showLivePlayable}
-								showTopBarDesktop={false}
-								showTopBarMobile={false}
-							/>
-						</li>
-					);
-				})}
-			</ScrollableCarousel>
-		</Island>
+		<ScrollableCarousel carouselLength={trails.length}>
+			{trails.map((trail) => {
+				return (
+					<li key={trail.url} css={[itemStyles, verticalLineStyles]}>
+						<FrontCard
+							trail={trail}
+							imageLoading={imageLoading}
+							absoluteServerTimes={!!absoluteServerTimes}
+							containerPalette={containerPalette}
+							containerType={containerType}
+							showAge={!!showAge}
+							headlineSizes={{ desktop: 'xxsmall' }}
+							imagePositionOnDesktop="left"
+							imagePositionOnMobile="left"
+							imageSize="small" // TODO - needs fixed width images
+							trailText={undefined} // unsupported
+							supportingContent={undefined} // unsupported
+							aspectRatio="5:4"
+							kickerText={trail.kickerText}
+							showLivePlayable={trail.showLivePlayable}
+							showTopBarDesktop={false}
+							showTopBarMobile={false}
+						/>
+					</li>
+				);
+			})}
+		</ScrollableCarousel>
 	);
 };
diff --git a/dotcom-rendering/src/components/ScrollableSmall.stories.tsx b/dotcom-rendering/src/components/ScrollableSmall.stories.tsx
@@ -3,7 +3,7 @@ import { discussionApiUrl } from '../../fixtures/manual/discussionApiUrl';
 import { trails } from '../../fixtures/manual/highlights-trails';
 import type { DCRContainerPalette } from '../types/front';
 import { FrontSection } from './FrontSection';
-import { ScrollableSmall } from './ScrollableSmall';
+import { ScrollableSmall } from './ScrollableSmall.importable';
 
 export default {
 	title: 'Components/ScrollableSmall',