Merge pull request #13579 from guardian/doml/card-gap-sizing-fix

Card gap sizing amendments

Author: domlander

dotcom-rendering/src/components/Card/Card.tsx

@@ -499,6 +499,7 @@ export const Card = ({
 		<div
 			css={css`
 				margin-top: auto;
+				display: flex;
 			`}
 		>
 			{isVideoArticle && (
@@ -626,51 +627,61 @@ export const Card = ({
 		}
 	};
 
-	/** Determines the gap of between card components based on card properties */
+	/**
+	 * Determines the gap of between card components based on card properties
+	 * Order matters here as the logic is based on the card properties
+	 */
 	const getGapSizes = (): GapSizes => {
 		if (isOnwardContent) {
 			return {
 				row: 'none',
 				column: 'none',
 			};
 		}
-		if (isMediaCardOrNewsletter && !isFlexibleContainer) {
-			return {
-				row: 'tiny',
-				column: 'tiny',
-			};
-		}
-		if (!!isFlexSplash || (isFlexibleContainer && imageSize === 'jumbo')) {
+
+		if (isFlexSplash) {
 			return {
 				row: 'small',
-				column: 'small',
+				column: 'none',
 			};
 		}
+
+		if (!isBetaContainer) {
+			/**
+			 * Media cards have 4px padding around the content so we have a
+			 * tiny (4px) gap to account for this and make it 8px total
+			 */
+			if (isMediaCardOrNewsletter) {
+				return {
+					row: 'tiny',
+					column: 'tiny',
+				};
+			}
+
+			// Current cards have small padding for everything
+			return { row: 'small', column: 'small' };
+		}
+
 		if (isSmallCard) {
 			return {
 				row: 'medium',
 				column: 'medium',
 			};
 		}
-		if (isBetaContainer && media?.type === 'avatar') {
-			return {
-				row: 'small',
-				column: 'small',
-			};
-		}
+
 		if (
-			isFlexibleContainer &&
-			(imagePositionOnDesktop === 'left' ||
-				imagePositionOnDesktop === 'right')
+			imagePositionOnDesktop === 'bottom' ||
+			imagePositionOnMobile === 'bottom'
 		) {
 			return {
-				row: 'large',
+				row: 'tiny',
 				column: 'large',
 			};
 		}
+
 		return {
-			row: isBetaContainer ? 'tiny' : 'small',
-			column: 'small',
+			row: 'small',
+			column: 'large',
 		};
 	};
 
@@ -1052,13 +1063,19 @@ export const Card = ({
 					<ContentWrapper
 						imageType={media?.type}
 						imageSize={imageSize}
+						isBetaContainer={isBetaContainer}
 						imagePositionOnDesktop={imagePositionOnDesktop}
+						imagePositionOnMobile={imagePositionOnMobile}
 						padContent={determinePadContent(
 							isMediaCardOrNewsletter,
 							isBetaContainer,
 							isOnwardContent,
 						)}
-						isFlexibleContainer={isFlexibleContainer}
+						padRight={
+							!!isFlexSplash &&
+							image &&
+							imagePositionOnDesktop === 'right'
+						}
 					>
 						{/* This div is needed to keep the headline and trail text justified at the start */}
 						<div

dotcom-rendering/src/components/Card/components/CardLayout.tsx

@@ -156,7 +156,7 @@ const decidePosition = (
 	`;
 };
 
-/** Detemines the gap size between components in card layout */
+/** Determines the gap size between components in card layout */
 const decideGap = (gapSize: GapSize) => {
 	switch (gapSize) {
 		case 'none':
@@ -172,6 +172,14 @@ const decideGap = (gapSize: GapSize) => {
 	}
 };
 
+const decideColumnGap = (gapSize: GapSize) => css`
+	column-gap: ${gapSize === 'large' ? '10px' : decideGap(gapSize)};
+
+	${from.tablet} {
+		column-gap: ${decideGap(gapSize)};
+	}
+`;
+
 export const CardLayout = ({
 	children,
 	cardBackgroundColour,
@@ -196,11 +204,11 @@ export const CardLayout = ({
 					isBetaContainer,
 					imageType === 'avatar',
 				),
+				decideColumnGap(gapSizes.column),
 			]}
 			style={{
 				backgroundColor: cardBackgroundColour,
 				rowGap: decideGap(gapSizes.row),
-				columnGap: decideGap(gapSizes.column),
 			}}
 		>
 			{children}

dotcom-rendering/src/components/Card/components/ContentWrapper.tsx

@@ -1,6 +1,6 @@
 import type { SerializedStyles } from '@emotion/react';
 import { css } from '@emotion/react';
-import { between, from, space } from '@guardian/source/foundations';
+import { between, from, space, until } from '@guardian/source/foundations';
 import type { CardImageType } from '../../../types/layout';
 import type { ImagePositionType, ImageSizeType } from './ImageWrapper';
 
@@ -14,7 +14,6 @@ const sizingStyles = css`
 /**
  * This function works in partnership with its sibling in `ImageWrapper`. If you
  * change any values here be sure to update that file as well.
- *
  */
 const flexBasisStyles = ({
 	imageSize,
@@ -61,67 +60,99 @@ const flexBasisStyles = ({
 	}
 };
 
-const paddingStyles = (
+type ImageDirection = 'vertical' | 'horizontal' | 'none';
+
+/**
+ * There is no padding on the side of the image where the text is.
+ */
+const paddingBetaContainerStyles = (
+	imagePositionMobile: ImagePositionType,
+	imagePositionDesktop: ImagePositionType,
+	padding: 1 | 2,
+) => css`
+	${until.tablet} {
+		padding-left: ${imagePositionMobile !== 'left' &&
+		`${space[padding]}px`};
+		padding-right: ${imagePositionMobile !== 'right' &&
+		`${space[padding]}px`};
+		padding-top: ${imagePositionMobile !== 'top' && `${space[padding]}px`};
+		padding-bottom: ${imagePositionMobile !== 'bottom' &&
+		`${space[padding]}px`};
+	}
+	${from.tablet} {
+		padding-left: ${imagePositionDesktop !== 'left' &&
+		`${space[padding]}px`};
+		padding-right: ${imagePositionDesktop !== 'right' &&
+		`${space[padding]}px`};
+		padding-top: ${imagePositionDesktop !== 'top' && `${space[padding]}px`};
+		padding-bottom: ${imagePositionDesktop !== 'bottom' &&
+		`${space[padding]}px`};
+	}
+`;
+
+const padRightStyles = css`
+	${from.tablet} {
+		padding-right: ${space[5]}px;
+	}
+`;
+
+const getImageDirection = (
 	imagePosition: ImagePositionType,
-	isFlexibleContainer: boolean,
-	paddingWidth: 1 | 2,
-) => {
-	/**
-	 * If we're in a flexible container there is a 20px gap between the image
-	 * and content. We don't apply padding to the content on the same edge as
-	 * the image so the content is aligned with the grid.
-	 */
-	if (isFlexibleContainer && imagePosition === 'left') {
-		return css`
-			padding: ${space[paddingWidth]}px ${space[paddingWidth]}px
-				${space[paddingWidth]}px 0;
-		`;
+): ImageDirection => {
+	if (imagePosition === 'top' || imagePosition === 'bottom') {
+		return 'vertical';
 	}
 
-	if (isFlexibleContainer && imagePosition === 'right') {
-		return css`
-			padding: ${space[paddingWidth]}px 0 ${space[paddingWidth]}px
-				${space[paddingWidth]}px;
-		`;
+	if (imagePosition === 'left' || imagePosition === 'right') {
+		return 'horizontal';
 	}
 
-	return css`
-		padding: ${space[paddingWidth]}px;
-	`;
+	return 'none';
 };
 
 type Props = {
 	children: React.ReactNode;
 	imageType?: CardImageType;
 	imageSize: ImageSizeType;
+	isBetaContainer: boolean;
 	imagePositionOnDesktop: ImagePositionType;
+	imagePositionOnMobile: ImagePositionType;
 	padContent?: 'small' | 'large';
-	isFlexibleContainer?: boolean;
+	padRight?: boolean;
 };
 
 export const ContentWrapper = ({
 	children,
 	imageType,
 	imageSize,
+	isBetaContainer,
 	imagePositionOnDesktop,
+	imagePositionOnMobile,
 	padContent,
-	isFlexibleContainer = false,
+	padRight = false,
 }: Props) => {
-	const isHorizontalOnDesktop =
-		imagePositionOnDesktop === 'left' || imagePositionOnDesktop === 'right';
+	const imageDirectionDesktop = getImageDirection(imagePositionOnDesktop);
+	const paddingSpace = padContent === 'small' ? 1 : 2;
 
 	return (
 		<div
 			css={[
 				sizingStyles,
-				isHorizontalOnDesktop &&
+				imageDirectionDesktop === 'horizontal' &&
 					flexBasisStyles({ imageSize, imageType }),
 				padContent &&
-					paddingStyles(
+					!isBetaContainer &&
+					css`
+						padding: ${space[paddingSpace]}px;
+					`,
+				padContent &&
+					isBetaContainer &&
+					paddingBetaContainerStyles(
+						imagePositionOnMobile,
 						imagePositionOnDesktop,
-						isFlexibleContainer,
-						padContent === 'small' ? 1 : 2,
+						paddingSpace,
 					),
+				padRight && padRightStyles,
 			]}
 		>
 			{children}

dotcom-rendering/src/components/Card/components/ImageWrapper.tsx

@@ -53,6 +53,27 @@ const imageOverlayContainerStyles = css`
 	height: 100%;
 `;
 
+/**
+ * There is no padding on the side of the image where the text is.
+ */
+const imagePaddingStyles = (
+	imagePositionOnDesktop: ImagePositionType,
+	imagePositionOnMobile: ImagePositionType,
+) => css`
+	${until.tablet} {
+		padding-left: ${imagePositionOnMobile !== 'right' && `${space[2]}px`};
+		padding-right: ${imagePositionOnMobile !== 'left' && `${space[2]}px`};
+		padding-top: ${imagePositionOnMobile !== 'bottom' && `${space[2]}px`};
+		padding-bottom: ${imagePositionOnMobile !== 'top' && `${space[2]}px`};
+	}
+	${from.tablet} {
+		padding-left: ${imagePositionOnDesktop !== 'right' && `${space[2]}px`};
+		padding-right: ${imagePositionOnDesktop !== 'left' && `${space[2]}px`};
+		padding-top: ${imagePositionOnDesktop !== 'bottom' && `${space[2]}px`};
+		padding-bottom: ${imagePositionOnDesktop !== 'top' && `${space[2]}px`};
+	}
+`;
+
 /**
  * This function works in partnership with its sibling in `ContentWrapper`. If you
  * change any values here be sure to update that file as well.
@@ -123,10 +144,6 @@ const fixImageWidth = ({
 	`}
 `;
 
-const imagePadding = css`
-	padding: ${space[2]}px;
-`;
-
 export const ImageWrapper = ({
 	children,
 	imageSize,
@@ -180,7 +197,11 @@ export const ImageWrapper = ({
 						display: block;
 					}
 				`,
-				padImage && imagePadding,
+				padImage &&
+					imagePaddingStyles(
+						imagePositionOnDesktop,
+						imagePositionOnMobile,
+					),
 			]}
 		>
 			<>
@@ -191,7 +212,11 @@ export const ImageWrapper = ({
 						<div
 							css={[
 								imageOverlayContainerStyles,
-								padImage && imagePadding,
+								padImage &&
+									imagePaddingStyles(
+										imagePositionOnDesktop,
+										imagePositionOnMobile,
+									),
 							]}
 						>
 							{/* This child div is needed as the hover background colour covers the padded