From 839eadce1e943c65055dec26acfd8e203a321ca2 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Wed, 28 Jun 2023 10:06:32 +0100 Subject: [PATCH 1/2] refactor(PlayIcon): narrow types reduce the number of possible branches by two, by being explicit about mobile sizes. Derive types from data using `as const` assertion --- .../components/Card/components/PlayIcon.tsx | 75 ++++++++----------- 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/dotcom-rendering/src/components/Card/components/PlayIcon.tsx b/dotcom-rendering/src/components/Card/components/PlayIcon.tsx index 1a6d95c6479..16e0ee16a1c 100644 --- a/dotcom-rendering/src/components/Card/components/PlayIcon.tsx +++ b/dotcom-rendering/src/components/Card/components/PlayIcon.tsx @@ -2,33 +2,14 @@ import { css } from '@emotion/react'; import { brandAlt, from } from '@guardian/source-foundations'; import { SvgMediaControlsPlay } from '@guardian/source-react-components'; -type PlayButtonSize = 'small' | 'medium' | 'large' | 'xlarge'; +type PlayButtonSize = keyof typeof sizes; -const buttonSize = (size: PlayButtonSize) => { - switch (size) { - case 'small': - return 28; - case 'medium': - return 44; - case 'large': - return 48; - case 'xlarge': - return 60; - } -}; - -const iconSize = (size: PlayButtonSize) => { - switch (size) { - case 'small': - return 26; - case 'medium': - return 36; - case 'large': - return 40; - case 'xlarge': - return 54; - } -}; +const sizes = { + small: { button: 28, icon: 26 }, + medium: { button: 44, icon: 36 }, + large: { button: 48, icon: 40 }, + xlarge: { button: 60, icon: 54 }, +} as const satisfies Record; const iconWrapperStyles = css` display: flex; /* Fixes the div mis-sizing itself */ @@ -37,14 +18,17 @@ const iconWrapperStyles = css` left: 4px; `; -const iconStyles = (size: PlayButtonSize, sizeOnMobile: PlayButtonSize) => css` +const iconStyles = ( + size: PlayButtonSize, + sizeOnMobile: Extract, +) => css` background-color: ${brandAlt[400]}; border-radius: 50%; - width: ${buttonSize(sizeOnMobile)}px; - height: ${buttonSize(sizeOnMobile)}px; + width: ${sizes[sizeOnMobile].button}px; + height: ${sizes[sizeOnMobile].button}px; ${from.tablet} { - width: ${buttonSize(size)}px; - height: ${buttonSize(size)}px; + width: ${sizes[size].button}px; + height: ${sizes[size].button}px; } display: flex; @@ -54,26 +38,29 @@ const iconStyles = (size: PlayButtonSize, sizeOnMobile: PlayButtonSize) => css` svg { /* Visual centering */ transform: translateX(1px); - width: ${iconSize(sizeOnMobile)}px; - height: ${iconSize(sizeOnMobile)}px; + width: ${sizes[sizeOnMobile].icon}px; + height: ${sizes[sizeOnMobile].icon}px; ${from.tablet} { - width: ${iconSize(size)}px; - height: ${iconSize(size)}px; + width: ${sizes[size].icon}px; + height: ${sizes[size].icon}px; } } `; -const getIconSizeOnDesktop = (imageSize: ImageSizeType): PlayButtonSize => { - if (imageSize === 'jumbo') return 'xlarge'; - else if (imageSize === 'large') return 'large'; - else if (imageSize === 'medium') return 'medium'; - else if (imageSize === 'small') return 'small'; - else return 'large'; +const getIconSizeOnDesktop = (imageSize: ImageSizeType) => { + switch (imageSize) { + case 'jumbo': + return 'xlarge'; + case 'large': + case 'carousel': + return 'large'; + case 'medium': + case 'small': + return imageSize; + } }; -const getIconSizeOnMobile = ( - imagePositionOnMobile: ImagePositionType, -): PlayButtonSize => +const getIconSizeOnMobile = (imagePositionOnMobile: ImagePositionType) => imagePositionOnMobile === 'left' || imagePositionOnMobile === 'right' ? 'small' : 'large'; From eafd8a0ac23c5570c6d1aef97cffc41abf98b35f Mon Sep 17 00:00:00 2001 From: Max Duval Date: Wed, 28 Jun 2023 14:22:00 +0100 Subject: [PATCH 2/2] refactor(ImageWrapper): extract types to component Make the relationship between where types are used and consumed explicit --- dotcom-rendering/index.d.ts | 4 ---- dotcom-rendering/src/components/Card/Card.tsx | 4 ++++ .../src/components/Card/components/AvatarContainer.tsx | 1 + .../src/components/Card/components/CardLayout.tsx | 1 + .../src/components/Card/components/ContentWrapper.tsx | 1 + .../src/components/Card/components/HeadlineWrapper.tsx | 1 + .../src/components/Card/components/ImageWrapper.tsx | 4 ++++ dotcom-rendering/src/components/Card/components/PlayIcon.tsx | 1 + .../src/components/Card/components/TrailTextWrapper.tsx | 1 + dotcom-rendering/src/components/CardPicture.tsx | 1 + dotcom-rendering/src/components/Slideshow.tsx | 1 + 11 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dotcom-rendering/index.d.ts b/dotcom-rendering/index.d.ts index aff55646336..2cc3669c7b1 100644 --- a/dotcom-rendering/index.d.ts +++ b/dotcom-rendering/index.d.ts @@ -307,10 +307,6 @@ interface FEKeyEventsRequest { filterKeyEvents: boolean; } -type ImagePositionType = 'left' | 'top' | 'right' | 'bottom' | 'none'; - -type ImageSizeType = 'small' | 'medium' | 'large' | 'jumbo' | 'carousel'; - type CardImageType = 'mainMedia' | 'avatar' | 'crossword' | 'slideshow'; type SmallHeadlineSize = diff --git a/dotcom-rendering/src/components/Card/Card.tsx b/dotcom-rendering/src/components/Card/Card.tsx index 28e3845727e..33bddb33a58 100644 --- a/dotcom-rendering/src/components/Card/Card.tsx +++ b/dotcom-rendering/src/components/Card/Card.tsx @@ -36,6 +36,10 @@ import { CardLink } from './components/CardLink'; import { CardWrapper } from './components/CardWrapper'; import { ContentWrapper } from './components/ContentWrapper'; import { HeadlineWrapper } from './components/HeadlineWrapper'; +import type { + ImagePositionType, + ImageSizeType, +} from './components/ImageWrapper'; import { ImageWrapper } from './components/ImageWrapper'; import { TrailTextWrapper } from './components/TrailTextWrapper'; diff --git a/dotcom-rendering/src/components/Card/components/AvatarContainer.tsx b/dotcom-rendering/src/components/Card/components/AvatarContainer.tsx index f8f72a4081d..1052fdb729f 100644 --- a/dotcom-rendering/src/components/Card/components/AvatarContainer.tsx +++ b/dotcom-rendering/src/components/Card/components/AvatarContainer.tsx @@ -1,5 +1,6 @@ import { css } from '@emotion/react'; import { from, space, until } from '@guardian/source-foundations'; +import type { ImagePositionType, ImageSizeType } from './ImageWrapper'; type Props = { children: React.ReactNode; diff --git a/dotcom-rendering/src/components/Card/components/CardLayout.tsx b/dotcom-rendering/src/components/Card/components/CardLayout.tsx index 643d35e69b6..2d06bb74fc4 100644 --- a/dotcom-rendering/src/components/Card/components/CardLayout.tsx +++ b/dotcom-rendering/src/components/Card/components/CardLayout.tsx @@ -1,5 +1,6 @@ import { css } from '@emotion/react'; import { until } from '@guardian/source-foundations'; +import type { ImagePositionType } from './ImageWrapper'; type Props = { children: React.ReactNode; diff --git a/dotcom-rendering/src/components/Card/components/ContentWrapper.tsx b/dotcom-rendering/src/components/Card/components/ContentWrapper.tsx index c9f8debd9de..4fac7cda29f 100644 --- a/dotcom-rendering/src/components/Card/components/ContentWrapper.tsx +++ b/dotcom-rendering/src/components/Card/components/ContentWrapper.tsx @@ -1,6 +1,7 @@ import type { SerializedStyles } from '@emotion/react'; import { css } from '@emotion/react'; import { between, from } from '@guardian/source-foundations'; +import type { ImagePositionType, ImageSizeType } from './ImageWrapper'; const sizingStyles = css` display: flex; diff --git a/dotcom-rendering/src/components/Card/components/HeadlineWrapper.tsx b/dotcom-rendering/src/components/Card/components/HeadlineWrapper.tsx index 908ba08aa0a..707dbde6ca1 100644 --- a/dotcom-rendering/src/components/Card/components/HeadlineWrapper.tsx +++ b/dotcom-rendering/src/components/Card/components/HeadlineWrapper.tsx @@ -1,5 +1,6 @@ import { css } from '@emotion/react'; import { from } from '@guardian/source-foundations'; +import type { ImagePositionType } from './ImageWrapper'; type Props = { children: React.ReactNode; diff --git a/dotcom-rendering/src/components/Card/components/ImageWrapper.tsx b/dotcom-rendering/src/components/Card/components/ImageWrapper.tsx index ec9287be0ce..9dd3ed5e986 100644 --- a/dotcom-rendering/src/components/Card/components/ImageWrapper.tsx +++ b/dotcom-rendering/src/components/Card/components/ImageWrapper.tsx @@ -3,6 +3,10 @@ import { css } from '@emotion/react'; import { between, from, until } from '@guardian/source-foundations'; import { PlayIcon } from './PlayIcon'; +export type ImagePositionType = 'left' | 'top' | 'right' | 'bottom' | 'none'; + +export type ImageSizeType = 'small' | 'medium' | 'large' | 'jumbo' | 'carousel'; + type Props = { children: React.ReactNode; imageSize: ImageSizeType; diff --git a/dotcom-rendering/src/components/Card/components/PlayIcon.tsx b/dotcom-rendering/src/components/Card/components/PlayIcon.tsx index 16e0ee16a1c..dfa0b0e9cf4 100644 --- a/dotcom-rendering/src/components/Card/components/PlayIcon.tsx +++ b/dotcom-rendering/src/components/Card/components/PlayIcon.tsx @@ -1,6 +1,7 @@ import { css } from '@emotion/react'; import { brandAlt, from } from '@guardian/source-foundations'; import { SvgMediaControlsPlay } from '@guardian/source-react-components'; +import type { ImagePositionType, ImageSizeType } from './ImageWrapper'; type PlayButtonSize = keyof typeof sizes; diff --git a/dotcom-rendering/src/components/Card/components/TrailTextWrapper.tsx b/dotcom-rendering/src/components/Card/components/TrailTextWrapper.tsx index 372f5d33951..2168df16b31 100644 --- a/dotcom-rendering/src/components/Card/components/TrailTextWrapper.tsx +++ b/dotcom-rendering/src/components/Card/components/TrailTextWrapper.tsx @@ -2,6 +2,7 @@ import { css } from '@emotion/react'; import { body, until } from '@guardian/source-foundations'; import { decidePalette } from '../../../lib/decidePalette'; import type { DCRContainerPalette } from '../../../types/front'; +import type { ImagePositionType, ImageSizeType } from './ImageWrapper'; type Props = { children: string | React.ReactNode; diff --git a/dotcom-rendering/src/components/CardPicture.tsx b/dotcom-rendering/src/components/CardPicture.tsx index 084589754b7..a6c0c540074 100644 --- a/dotcom-rendering/src/components/CardPicture.tsx +++ b/dotcom-rendering/src/components/CardPicture.tsx @@ -1,6 +1,7 @@ import { css } from '@emotion/react'; import { breakpoints } from '@guardian/source-foundations'; import React from 'react'; +import type { ImageSizeType } from './Card/components/ImageWrapper'; import type { ImageWidthType } from './Picture'; import { generateSources, getFallbackSource } from './Picture'; diff --git a/dotcom-rendering/src/components/Slideshow.tsx b/dotcom-rendering/src/components/Slideshow.tsx index 96a38b9cab6..c6e21211caa 100644 --- a/dotcom-rendering/src/components/Slideshow.tsx +++ b/dotcom-rendering/src/components/Slideshow.tsx @@ -1,6 +1,7 @@ import { css, keyframes } from '@emotion/react'; import { from, neutral, textSans, until } from '@guardian/source-foundations'; import type { DCRSlideshowImage } from '../types/front'; +import type { ImageSizeType } from './Card/components/ImageWrapper'; import { CardPicture } from './CardPicture'; /**