From 8ea77fc9bdb0e8998637a92814cee9f45f442ee3 Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Mon, 7 Dec 2020 09:16:50 +0000 Subject: [PATCH 1/6] Don't choose immersive main media widths based on viewport --- src/web/components/Img.tsx | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/web/components/Img.tsx b/src/web/components/Img.tsx index 816a1a30ce8..3de32ac514f 100644 --- a/src/web/components/Img.tsx +++ b/src/web/components/Img.tsx @@ -48,6 +48,17 @@ const buildSourcesString = (srcSets: SrcSetItem[]): string => { return srcSets.map((srcSet) => `${srcSet.src} ${srcSet.width}w`).join(','); }; +/** + * mobile: 320 + * mobileMedium: 375 + * mobileLandscape: 480 + * phablet: 660 + * tablet: 740 + * desktop: 980 + * leftCol: 1140 + * wide: 1300 + */ + const buildSizesString = (role: RoleType, isMainMedia: boolean): string => { switch (role) { case 'inline': @@ -57,8 +68,25 @@ const buildSizesString = (role: RoleType, isMainMedia: boolean): string => { case 'thumbnail': return '140px'; case 'immersive': + // Immersive MainMedia elements fill the height of the viewport, meaning + // on mobile devices even though the viewport width is small, we'll need + // a larger image to maintain quality. Deciding which width image to ask + // for is difficult because aspect ratios and orientations between + // different devices varies so the values given below are best estimates + // based on the most popular configurations + + // Immersive body images stretch the full viewport width below wide, + // but do not stretch beyond 1300px after that. return isMainMedia - ? '100vw' + ? ` + (min-width: ${breakpoints.leftCol}px) 1600px, + (min-width: ${breakpoints.desktop}px) 1900px, + (min-width: ${breakpoints.tablet}px) 1900px, + (min-width: ${breakpoints.phablet}px) 1600px, + (min-width: ${breakpoints.mobileLandscape}px) 1300px, + (min-width: ${breakpoints.mobileMedium}px) 900px, + 1300px + ` : `(min-width: ${breakpoints.wide}px) 1300px, 100vw`; case 'supporting': return `(min-width: ${breakpoints.wide}px) 380px, 300px`; From 8c9969021c61f798fa85eb06cc149910dd7dc17b Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Mon, 7 Dec 2020 23:39:46 +0000 Subject: [PATCH 2/6] Use orientation portrait and vh to work out width --- src/web/components/Img.tsx | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/web/components/Img.tsx b/src/web/components/Img.tsx index 3de32ac514f..14350e242a4 100644 --- a/src/web/components/Img.tsx +++ b/src/web/components/Img.tsx @@ -78,15 +78,7 @@ const buildSizesString = (role: RoleType, isMainMedia: boolean): string => { // Immersive body images stretch the full viewport width below wide, // but do not stretch beyond 1300px after that. return isMainMedia - ? ` - (min-width: ${breakpoints.leftCol}px) 1600px, - (min-width: ${breakpoints.desktop}px) 1900px, - (min-width: ${breakpoints.tablet}px) 1900px, - (min-width: ${breakpoints.phablet}px) 1600px, - (min-width: ${breakpoints.mobileLandscape}px) 1300px, - (min-width: ${breakpoints.mobileMedium}px) 900px, - 1300px - ` + ? `(orientation: portrait) 167vh, 100vw` : `(min-width: ${breakpoints.wide}px) 1300px, 100vw`; case 'supporting': return `(min-width: ${breakpoints.wide}px) 380px, 300px`; From 6cf92010196d15ff673d722e3523f99751c55005 Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Tue, 8 Dec 2020 11:13:49 +0000 Subject: [PATCH 3/6] Update src/web/components/Img.tsx Co-authored-by: Gareth Trufitt --- src/web/components/Img.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/web/components/Img.tsx b/src/web/components/Img.tsx index 14350e242a4..999388d4e2c 100644 --- a/src/web/components/Img.tsx +++ b/src/web/components/Img.tsx @@ -74,6 +74,9 @@ const buildSizesString = (role: RoleType, isMainMedia: boolean): string => { // for is difficult because aspect ratios and orientations between // different devices varies so the values given below are best estimates // based on the most popular configurations + // + // The value of 167vh relates to a 5:3 image which is equal to 167 (viewport height) : 100 (viewport width). + // // Immersive body images stretch the full viewport width below wide, // but do not stretch beyond 1300px after that. From 6aaf1d521ed6a7f69d1cddd441d2fb981d3325e6 Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Tue, 8 Dec 2020 11:17:11 +0000 Subject: [PATCH 4/6] Updated comment for 167vh approach --- src/web/components/Img.tsx | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/web/components/Img.tsx b/src/web/components/Img.tsx index 999388d4e2c..c28936e075c 100644 --- a/src/web/components/Img.tsx +++ b/src/web/components/Img.tsx @@ -70,13 +70,10 @@ const buildSizesString = (role: RoleType, isMainMedia: boolean): string => { case 'immersive': // Immersive MainMedia elements fill the height of the viewport, meaning // on mobile devices even though the viewport width is small, we'll need - // a larger image to maintain quality. Deciding which width image to ask - // for is difficult because aspect ratios and orientations between - // different devices varies so the values given below are best estimates - // based on the most popular configurations - // - // The value of 167vh relates to a 5:3 image which is equal to 167 (viewport height) : 100 (viewport width). - // + // a larger image to maintain quality. To solve this problem we're using + // the viewport height (vh) to calculate width. The value of 167vh + // relates to an assumed image ratio of 5:3 which is equal to + // 167 (viewport height) : 100 (viewport width). // Immersive body images stretch the full viewport width below wide, // but do not stretch beyond 1300px after that. From 28520b83c828aee9b58b78dfab5b4be9d62794f6 Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Tue, 8 Dec 2020 11:49:38 +0000 Subject: [PATCH 5/6] Reduce Chromatic check threashold for these image stories --- src/web/components/elements/ImageBlockComponent.stories.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/web/components/elements/ImageBlockComponent.stories.tsx b/src/web/components/elements/ImageBlockComponent.stories.tsx index 1cb23da3492..8df69f3dea3 100644 --- a/src/web/components/elements/ImageBlockComponent.stories.tsx +++ b/src/web/components/elements/ImageBlockComponent.stories.tsx @@ -14,6 +14,9 @@ import { image } from './ImageBlockComponent.mocks'; export default { component: ImageBlockComponent, title: 'Components/ImageBlockComponent', + parameters: { + chromatic: { diffThreshold: 0.2 }, + }, }; const Container = ({ children }: { children: JSX.Element | JSX.Element[] }) => ( From e39fa8afd23a3b4f030ab2c3a5bfa37d01d76326 Mon Sep 17 00:00:00 2001 From: Oliver Lloyd Date: Tue, 8 Dec 2020 12:18:54 +0000 Subject: [PATCH 6/6] Increase threshold for these images --- src/web/components/elements/ImageBlockComponent.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/web/components/elements/ImageBlockComponent.stories.tsx b/src/web/components/elements/ImageBlockComponent.stories.tsx index 8df69f3dea3..b0ba24e9131 100644 --- a/src/web/components/elements/ImageBlockComponent.stories.tsx +++ b/src/web/components/elements/ImageBlockComponent.stories.tsx @@ -15,7 +15,7 @@ export default { component: ImageBlockComponent, title: 'Components/ImageBlockComponent', parameters: { - chromatic: { diffThreshold: 0.2 }, + chromatic: { diffThreshold: 0.4 }, }, };