From b824079c22fc7eb2e0fa8463ca48015a90a06cdb Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Sun, 15 Nov 2020 00:48:33 +0000 Subject: [PATCH 01/10] Move image source logic down into the Picture component --- src/web/components/Picture.tsx | 101 ++++++++++++++++-- .../components/elements/ImageComponent.tsx | 91 ++-------------- 2 files changed, 101 insertions(+), 91 deletions(-) diff --git a/src/web/components/Picture.tsx b/src/web/components/Picture.tsx index 890c822debc..ea41ca56644 100644 --- a/src/web/components/Picture.tsx +++ b/src/web/components/Picture.tsx @@ -1,7 +1,16 @@ import React from 'react'; // IE 9 needs this workaround as per https://scottjehl.github.io/picturefill/ -export interface PictureSource { +type Props = { + imageSources: ImageSource[]; + role: RoleType; + alt: string; + height: string; + width: string; + isLazy?: boolean; +}; + +interface PictureSource { width: number; minWidth: number; srcset: string; @@ -18,14 +27,88 @@ const forSource: (source: PictureSource) => string = (source) => source.srcset }" />`; -export const Picture: React.FC<{ - sources: PictureSource[]; - alt: string; - src: string; - height: string; - width: string; - isLazy?: boolean; -}> = ({ sources, alt, src, height, width, isLazy = true }) => { +const selectScrSetItemForWidth = ( + desiredWidth: number, + inlineSrcSets: SrcSetItem[], +): SrcSetItem => { + const sorted = inlineSrcSets.sort((a, b) => b.width - a.width); + + return sorted.reduce((best, current) => { + if (current.width < best.width && current.width >= desiredWidth) { + return current; + } + + return best; + }); +}; + +const getSrcSetsForWeighting = ( + imageSources: ImageSource[], + forWeighting: RoleType, +): SrcSetItem[] => + imageSources.filter( + ({ weighting }) => + // Use toLowerCase to handle cases where we have halfWidth comparing to halfwidth + weighting.toLowerCase() === forWeighting.toLowerCase(), + )[0].srcSet; + +const makePictureSource = ( + hidpi: boolean, + minWidth: number, + srcSet: SrcSetItem, +): PictureSource => { + return { + hidpi, + minWidth, + width: srcSet.width, + srcset: `${srcSet.src} ${hidpi ? srcSet.width * 2 : srcSet.width}w`, + }; +}; + +const makeSources = ( + imageSources: ImageSource[], + role: RoleType, +): PictureSource[] => { + const inlineSrcSets = getSrcSetsForWeighting(imageSources, role); + const sources: PictureSource[] = []; + inlineSrcSets + .map((item) => item.width) + .forEach((width) => { + sources.push( + makePictureSource( + true, + width, + selectScrSetItemForWidth(width, inlineSrcSets), + ), + ); + sources.push( + makePictureSource( + false, + width, + selectScrSetItemForWidth(width, inlineSrcSets), + ), + ); + }); + + return sources; +}; + +const getFallback: (imageSources: ImageSource[]) => string = (imageSources) => { + const inlineSrcSets = getSrcSetsForWeighting(imageSources, 'inline'); + + return selectScrSetItemForWidth(300, inlineSrcSets).src; +}; + +export const Picture = ({ + imageSources, + role, + alt, + height, + width, + isLazy = true, +}: Props) => { + const sources = makeSources(imageSources, role); + const src = getFallback(imageSources); return ( // https://stackoverflow.com/questions/10844205/html-5-strange-img-always-adds-3px-margin-at-bottom // why did we put `style="vertical-align: middle;"` inside the img tag diff --git a/src/web/components/elements/ImageComponent.tsx b/src/web/components/elements/ImageComponent.tsx index 5d19524b354..43f412b5f5b 100644 --- a/src/web/components/elements/ImageComponent.tsx +++ b/src/web/components/elements/ImageComponent.tsx @@ -6,7 +6,7 @@ import { headline } from '@guardian/src-foundations/typography'; import { pillarPalette } from '@root/src/lib/pillars'; import { brandAltBackground, neutral } from '@guardian/src-foundations/palette'; -import { Picture, PictureSource } from '@root/src/web/components/Picture'; +import { Picture } from '@root/src/web/components/Picture'; import { Caption } from '@root/src/web/components/Caption'; import { Hide } from '@root/src/web/components/Hide'; import { StarRating } from '@root/src/web/components/StarRating/StarRating'; @@ -24,78 +24,6 @@ type Props = { title?: string; }; -const selectScrSetItemForWidth = ( - desiredWidth: number, - inlineSrcSets: SrcSetItem[], -): SrcSetItem => { - const sorted = inlineSrcSets.sort((a, b) => b.width - a.width); - - return sorted.reduce((best, current) => { - if (current.width < best.width && current.width >= desiredWidth) { - return current; - } - - return best; - }); -}; - -const getSrcSetsForWeighting = ( - imageSources: ImageSource[], - forWeighting: RoleType, -): SrcSetItem[] => - imageSources.filter( - ({ weighting }) => - // Use toLowerCase to handle cases where we have halfWidth comparing to halfwidth - weighting.toLowerCase() === forWeighting.toLowerCase(), - )[0].srcSet; - -const makePictureSource = ( - hidpi: boolean, - minWidth: number, - srcSet: SrcSetItem, -): PictureSource => { - return { - hidpi, - minWidth, - width: srcSet.width, - srcset: `${srcSet.src} ${hidpi ? srcSet.width * 2 : srcSet.width}w`, - }; -}; - -const makeSources = ( - imageSources: ImageSource[], - role: RoleType, -): PictureSource[] => { - const inlineSrcSets = getSrcSetsForWeighting(imageSources, role); - const sources: PictureSource[] = []; - inlineSrcSets - .map((item) => item.width) - .forEach((width) => { - sources.push( - makePictureSource( - true, - width, - selectScrSetItemForWidth(width, inlineSrcSets), - ), - ); - sources.push( - makePictureSource( - false, - width, - selectScrSetItemForWidth(width, inlineSrcSets), - ), - ); - }); - - return sources; -}; - -const getFallback: (imageSources: ImageSource[]) => string = (imageSources) => { - const inlineSrcSets = getSrcSetsForWeighting(imageSources, 'inline'); - - return selectScrSetItemForWidth(300, inlineSrcSets).src; -}; - const starsWrapper = css` background-color: ${brandAltBackground.primary}; @@ -291,8 +219,6 @@ export const ImageComponent = ({ starRating, title, }: Props) => { - const { imageSources } = element; - const sources = makeSources(imageSources, role); const shouldLimitWidth = !isMainMedia && (role === 'showcase' || role === 'supporting' || role === 'immersive'); @@ -333,9 +259,9 @@ export const ImageComponent = ({ `} > - + {/* CaptionToggle contains the input with id #the-checkbox */} + {' '}
Date: Mon, 16 Nov 2020 09:24:27 +0000 Subject: [PATCH 02/10] Replace picture tag with an img tag --- src/web/components/Picture.tsx | 153 ++++++++++++++++----------------- 1 file changed, 72 insertions(+), 81 deletions(-) diff --git a/src/web/components/Picture.tsx b/src/web/components/Picture.tsx index ea41ca56644..99dbe8d6098 100644 --- a/src/web/components/Picture.tsx +++ b/src/web/components/Picture.tsx @@ -1,5 +1,5 @@ import React from 'react'; -// IE 9 needs this workaround as per https://scottjehl.github.io/picturefill/ +import { css } from 'emotion'; type Props = { imageSources: ImageSource[]; @@ -7,96 +7,81 @@ type Props = { alt: string; height: string; width: string; + isMainMedia?: boolean; isLazy?: boolean; }; -interface PictureSource { - width: number; - minWidth: number; - srcset: string; - hidpi: boolean; -} - -const mq: (source: PictureSource) => string = (source) => - source.hidpi - ? `(min-width: ${source.minWidth}px) and (-webkit-min-device-pixel-ratio: 1.25), (min-width: ${source.minWidth}px) and (min-resolution: 120dpi)` - : `(min-width: ${source.minWidth}px)`; - -const forSource: (source: PictureSource) => string = (source) => - ` `; - -const selectScrSetItemForWidth = ( +const getClosestSetForWidth = ( desiredWidth: number, inlineSrcSets: SrcSetItem[], ): SrcSetItem => { + // For a desired width, find the SrcSetItem which is the closest match const sorted = inlineSrcSets.sort((a, b) => b.width - a.width); - return sorted.reduce((best, current) => { if (current.width < best.width && current.width >= desiredWidth) { return current; } - return best; }); }; -const getSrcSetsForWeighting = ( +const getFallbackSrc = (srcSets: SrcSetItem[]): string => { + // The assumption here is readers on devices that do not support srcset are likely to be on poor + // network connections so we're going to fallback to a small image + return getClosestSetForWidth(300, srcSets).src; +}; + +// const getSrcSetsForRole = ( +// role: RoleType, +// imageSources: ImageSource[], +// ): SrcSetItem[] | undefined => { +// return imageSources.find((source) => source.weighting === role)?.srcSet; +// }; + +const getSrcSetsForRole = ( + role: RoleType, imageSources: ImageSource[], - forWeighting: RoleType, -): SrcSetItem[] => - imageSources.filter( +): SrcSetItem[] => { + return imageSources.filter( ({ weighting }) => // Use toLowerCase to handle cases where we have halfWidth comparing to halfwidth - weighting.toLowerCase() === forWeighting.toLowerCase(), + weighting.toLowerCase() === role.toLowerCase(), )[0].srcSet; - -const makePictureSource = ( - hidpi: boolean, - minWidth: number, - srcSet: SrcSetItem, -): PictureSource => { - return { - hidpi, - minWidth, - width: srcSet.width, - srcset: `${srcSet.src} ${hidpi ? srcSet.width * 2 : srcSet.width}w`, - }; }; -const makeSources = ( - imageSources: ImageSource[], - role: RoleType, -): PictureSource[] => { - const inlineSrcSets = getSrcSetsForWeighting(imageSources, role); - const sources: PictureSource[] = []; - inlineSrcSets - .map((item) => item.width) - .forEach((width) => { - sources.push( - makePictureSource( - true, - width, - selectScrSetItemForWidth(width, inlineSrcSets), - ), - ); - sources.push( - makePictureSource( - false, - width, - selectScrSetItemForWidth(width, inlineSrcSets), - ), - ); - }); - - return sources; +const buildSourcesString = (srcSets: SrcSetItem[]): string => { + return srcSets.map((srcSet) => `${srcSet.src} ${srcSet.width}w`).join(','); }; -const getFallback: (imageSources: ImageSource[]) => string = (imageSources) => { - const inlineSrcSets = getSrcSetsForWeighting(imageSources, 'inline'); +const buildSizesString = (role: RoleType, isMainMedia: boolean): string => { + const breakpoints = { + mobileMedium: 375, + mobileLandscape: 480, + phablet: 660, + tablet: 740, + desktop: 980, + leftCol: 1140, + wide: 1300, + }; - return selectScrSetItemForWidth(300, inlineSrcSets).src; + switch (role) { + case 'inline': + return `(min-width: ${breakpoints.phablet}px) 620px, 100vw`; + case 'halfWidth': + return `(min-width: ${breakpoints.phablet}px) 300px, 50vw`; + case 'thumbnail': + return '140px'; + case 'immersive': + return isMainMedia + ? '100vw' + : `(min-width: ${breakpoints.wide}px) 1300px, 100vw`; + case 'supporting': + return `(min-width: ${breakpoints.wide}px) 380px, 300px`; + case 'showcase': + return isMainMedia + ? `(min-width: ${breakpoints.wide}px) 1020px, (min-width: ${breakpoints.leftCol}px) 940px, (min-width: ${breakpoints.tablet}px) 700px, (min-width: ${breakpoints.phablet}px) 660px, 100vw` + : `(min-width: ${breakpoints.wide}px) 860px, (min-width: ${breakpoints.leftCol}px) 780px, (min-width: ${breakpoints.phablet}px) 620px, 10vw`; + } }; export const Picture = ({ @@ -105,23 +90,29 @@ export const Picture = ({ alt, height, width, + isMainMedia = false, isLazy = true, }: Props) => { - const sources = makeSources(imageSources, role); - const src = getFallback(imageSources); + const srcSetForRole = getSrcSetsForRole(role, imageSources); + const src = getFallbackSrc(srcSetForRole); + const sources = buildSourcesString(srcSetForRole); + const sizes = buildSizesString(role, isMainMedia); + return ( - // https://stackoverflow.com/questions/10844205/html-5-strange-img-always-adds-3px-margin-at-bottom - // why did we put `style="vertical-align: middle;"` inside the img tag -