Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support fixing image sizes at tablet and desktop #12572

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
27 changes: 26 additions & 1 deletion dotcom-rendering/src/components/Card/Card.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1509,7 +1509,7 @@ export const WithNoGap = () => {
);
};

export const WithASmallGap = () => {
export const WithATinyGap = () => {
return (
<>
<CardWrapper>
Expand All @@ -1533,6 +1533,30 @@ export const WithASmallGap = () => {
);
};

export const WithASmallGap = () => {
return (
<>
<CardWrapper>
<div
css={css`
width: 280px;
`}
>
<Card
{...basicCardProps}
imagePositionOnDesktop="left"
format={{
display: ArticleDisplay.Standard,
design: ArticleDesign.Comment,
theme: Pillar.Opinion,
}}
/>
</div>
</CardWrapper>
</>
);
};

export const WithAMediumGap = () => {
return (
<>
Expand All @@ -1544,6 +1568,7 @@ export const WithAMediumGap = () => {
>
<Card
{...basicCardProps}
containerType={'scrollable/small'}
imagePositionOnDesktop="left"
format={{
display: ArticleDisplay.Standard,
Expand Down
24 changes: 20 additions & 4 deletions dotcom-rendering/src/components/Card/Card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { CardWrapper } from './components/CardWrapper';
import { ContentWrapper } from './components/ContentWrapper';
import { HeadlineWrapper } from './components/HeadlineWrapper';
import type {
ImageFixedSizeOptions,
ImagePositionType,
ImageSizeType,
} from './components/ImageWrapper';
Expand Down Expand Up @@ -437,6 +438,20 @@ export const Card = ({
containerType === 'flexible/special' ||
containerType === 'flexible/general';

const isSmallCard = containerType === 'scrollable/small';

const imageFixedSizeOptions = (): ImageFixedSizeOptions => {
if (isSmallCard) {
return {
mobile: 'tiny',
tablet: 'small',
desktop: 'small',
};
}
if (isFlexibleContainer) return { mobile: 'small' };
return { mobile: 'medium' };
};

const headlinePosition = getHeadlinePosition({
containerType,
isFlexSplash,
Expand All @@ -446,16 +461,17 @@ export const Card = ({
/** Determines the gap of between card components based on card properties */
const getGapSize = (): GapSize => {
if (isOnwardContent) return 'none';
if (hasBackgroundColour) return 'small';
if (isFlexSplash) return 'medium';
if (hasBackgroundColour) return 'tiny';
if (isFlexSplash) return 'small';
if (isSmallCard) return 'medium';
if (
isFlexibleContainer &&
(imagePositionOnDesktop === 'left' ||
imagePositionOnDesktop === 'right')
) {
return 'large';
}
return 'medium';
return 'small';
};

/**
Expand Down Expand Up @@ -579,11 +595,11 @@ export const Card = ({
{media && (
<ImageWrapper
imageSize={imageSize}
imageFixedSizes={imageFixedSizeOptions()}
imageType={media.type}
imagePositionOnDesktop={imagePositionOnDesktop}
imagePositionOnMobile={imagePositionOnMobile}
showPlayIcon={showPlayIcon}
isFlexibleContainer={isFlexibleContainer}
>
{media.type === 'slideshow' && (
<Slideshow
Expand Down
10 changes: 6 additions & 4 deletions dotcom-rendering/src/components/Card/components/CardLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { ImagePositionType } from './ImageWrapper';

const padding = 20;

export type GapSize = 'none' | 'small' | 'medium' | 'large';
export type GapSize = 'none' | 'tiny' | 'small' | 'medium' | 'large';

type Props = {
children: React.ReactNode;
Expand Down Expand Up @@ -103,10 +103,12 @@ const decideGap = (gapSize: GapSize) => {
switch (gapSize) {
case 'none':
return `0`;
case 'small':
case 'tiny':
return `${space[1]}px`;
case 'medium':
case 'small':
return `${space[2]}px`;
case 'medium':
return '10px';
case 'large':
return `${space[5]}px`;
}
Expand All @@ -120,7 +122,7 @@ export const CardLayout = ({
minWidthInPixels,
imageType,
containerType,
gapSize = 'medium',
gapSize = 'small',
}: Props) => (
<div
css={[
Expand Down
60 changes: 48 additions & 12 deletions dotcom-rendering/src/components/Card/components/ImageWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,31 @@ 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';
const imageFixedSize = {
tiny: 86,
small: 122.5,
medium: 125,
Comment on lines +8 to +9
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names feel a little awkward given that the sizes are now so close (small was previously 97.5). I wonder if we could get away with making 122.5 the default fixed size everywhere in place of 125?

};

type ImageFixedSize = keyof typeof imageFixedSize;

export type ImageFixedSizeOptions = {
mobile: ImageFixedSize;
tablet?: ImageFixedSize;
desktop?: ImageFixedSize;
};

export type ImagePositionType = 'left' | 'top' | 'right' | 'bottom' | 'none';
export type ImageSizeType = 'small' | 'medium' | 'large' | 'jumbo' | 'carousel';

type Props = {
children: React.ReactNode;
imageSize: ImageSizeType;
imageFixedSizes?: ImageFixedSizeOptions;
imageType?: CardImageType;
imagePositionOnDesktop: ImagePositionType;
imagePositionOnMobile: ImagePositionType;
showPlayIcon: boolean;
/** Flexible containers require different styling */
isFlexibleContainer?: boolean;
};

/**
Expand Down Expand Up @@ -54,25 +66,49 @@ const flexBasisStyles = ({
`;
}
};
/** Below tablet, we fix the size of the image and add a margin
around it. The corresponding content flex grows to fill the space */
const fixedImageWidth = (isFlexibleContainer: boolean) => css`
/**
* Below tablet, we fix the size of the image and add a margin around it.
* The corresponding content flex grows to fill the space.
*
* Fixed images sizes can optionally be applied at tablet and desktop.
*/
const fixImageWidthStyles = (width: number) => css`
width: ${width}px;
flex-shrink: 0;
flex-basis: unset;
align-self: flex-start;
`;

const fixImageWidth = ({
mobile,
tablet,
desktop,
}: ImageFixedSizeOptions) => css`
${until.tablet} {
width: ${isFlexibleContainer ? '97.5px' : '125px'};
flex-shrink: 0;
flex-basis: unset;
align-self: flex-start;
${fixImageWidthStyles(imageFixedSize[mobile])}
}
${tablet &&
css`
${between.tablet.and.desktop} {
${fixImageWidthStyles(imageFixedSize[tablet])}
}
`}
${desktop &&
css`
${from.desktop} {
${fixImageWidthStyles(imageFixedSize[desktop])}
}
`}
`;

export const ImageWrapper = ({
children,
imageSize,
imageFixedSizes = { mobile: 'medium' },
imageType,
imagePositionOnDesktop,
imagePositionOnMobile,
showPlayIcon,
isFlexibleContainer = false,
}: Props) => {
const isHorizontalOnDesktop =
imagePositionOnDesktop === 'left' || imagePositionOnDesktop === 'right';
Expand Down Expand Up @@ -103,7 +139,7 @@ export const ImageWrapper = ({
display: none;
}
`,
isHorizontalOnMobile && fixedImageWidth(isFlexibleContainer),
isHorizontalOnMobile && fixImageWidth(imageFixedSizes),

isHorizontalOnDesktop &&
css`
Expand Down
12 changes: 10 additions & 2 deletions dotcom-rendering/src/components/ScrollableSmall.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { breakpoints } from '@guardian/source/foundations';
import type { Meta, StoryObj } from '@storybook/react';
import { discussionApiUrl } from '../../fixtures/manual/discussionApiUrl';
import { trails } from '../../fixtures/manual/highlights-trails';
Expand All @@ -8,6 +9,15 @@ import { ScrollableSmall } from './ScrollableSmall.importable';
export default {
title: 'Components/ScrollableSmall',
component: ScrollableSmall,
parameters: {
chromatic: {
viewports: [
breakpoints.mobile,
breakpoints.tablet,
breakpoints.wide,
],
},
},
args: {
trails,
containerPalette: undefined,
Expand All @@ -20,8 +30,6 @@ export default {

type Story = StoryObj<typeof ScrollableSmall>;

export const Default = {};

export const WithFrontSection = {
render: (args) => (
<FrontSection
Expand Down
Loading