-
Notifications
You must be signed in to change notification settings - Fork 258
WS-1674 Implement Temporary UX Solution for Portrait Video Promo Display on Index Pages #13432
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
base: latest
Are you sure you want to change the base?
Changes from 18 commits
c27fe8b
1625b1b
95dffb3
f49add0
4f4886e
7b630d5
154c975
cae21d3
c35f5a2
72bcb8c
6269515
6182c31
581ea37
f5a3c88
5ba9a81
4e2d44a
2de54b5
899ceb0
741bda6
0358f97
501c66b
4b06899
d631a25
693cfd2
0e0bd24
23164e3
58f222c
8c83113
eafae85
a048344
e48de6c
e68ff73
74b8bd4
3a09577
3c8f771
17b1b0d
3f0d5b2
826c9cb
ca0a81d
fabf938
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,15 @@ | ||
| /** @jsx jsx */ | ||
| /* @jsxFrag React.Fragment */ | ||
| import React, { Fragment, PropsWithChildren, useState, use } from 'react'; | ||
| import { Global, jsx } from '@emotion/react'; | ||
| import React, { | ||
| Fragment, | ||
| PropsWithChildren, | ||
| useState, | ||
| use, | ||
| useEffect, | ||
| } from 'react'; | ||
| import { Global, jsx, useTheme } from '@emotion/react'; | ||
| import { Helmet } from 'react-helmet'; | ||
| import useImageColour from '#app/hooks/useImageColour'; | ||
| import styles from './index.styles'; | ||
| import { RequestContext } from '../../contexts/RequestContext'; | ||
| import { HOME_PAGE } from '../../routes/utils/pageTypes'; | ||
|
|
@@ -27,6 +34,7 @@ export type ImageProps = { | |
| fetchPriority?: 'high'; | ||
| hasCaption?: boolean; | ||
| isPortraitOrientation?: boolean; | ||
| isCurationPromo?: boolean; | ||
| }; | ||
|
|
||
| const roundNumber = (num: number) => Math.round(num * 100) / 100; | ||
|
|
@@ -56,9 +64,32 @@ const Image = ({ | |
| fetchPriority, | ||
| hasCaption, | ||
| isPortraitOrientation, | ||
| isCurationPromo, | ||
| }: PropsWithChildren<ImageProps>) => { | ||
| const { pageType, isLite, isAmp } = use(RequestContext); | ||
| const [isLoaded, setIsLoaded] = useState(false); | ||
| const [isPortrait, setIsPortrait] = useState(true); | ||
emilysaffron marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const imgRef = React.useRef<HTMLImageElement | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| const img = imgRef.current; | ||
| if (img?.complete) { | ||
| setIsPortrait(img.naturalHeight > img.naturalWidth); | ||
| setIsLoaded(true); | ||
| } | ||
| }, []); | ||
emilysaffron marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const { | ||
| palette: { GREY_8 }, | ||
| } = useTheme(); | ||
|
|
||
| const { isLoading, colour } = useImageColour(src, { | ||
| fallbackColour: GREY_8, | ||
| minimumContrast: 0, | ||
| contrastColour: '#ffffff', | ||
emilysaffron marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| paletteSize: 10, | ||
emilysaffron marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }); | ||
|
|
||
| if (isLite) return null; | ||
|
|
||
| const showPlaceholder = placeholder && !isLoaded; | ||
|
|
@@ -164,8 +195,25 @@ const Image = ({ | |
| /> | ||
| </> | ||
| )} | ||
| {isPortrait && isCurationPromo && ( | ||
| <div | ||
| css={[ | ||
| styles.gradientBackground, | ||
| { | ||
| background: colour | ||
| ? `linear-gradient(200deg,rgba(${colour.rgb.join(',')}, 0.6) 0%, #180109 54%, #180109 90%)` | ||
| : `linear-gradient(200deg, ${GREY_8} 0%, #180109 54%, #180109 90%)`, | ||
emilysaffron marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }, | ||
| ]} | ||
| /> | ||
| )} | ||
| <img | ||
| onLoad={() => setIsLoaded(true)} | ||
| ref={imgRef} | ||
| onLoad={e => { | ||
| const img = e.currentTarget; | ||
| setIsPortrait(img.naturalHeight > img.naturalWidth); | ||
| setIsLoaded(true); | ||
| }} | ||
| src={src} | ||
| {...(srcSet && { srcSet: imgSrcSet })} | ||
| {...(imgSizes && { sizes: imgSizes })} | ||
|
|
@@ -174,10 +222,21 @@ const Image = ({ | |
| width={width} | ||
| height={height} | ||
| css={[ | ||
| styles.image, | ||
| isPortrait && isCurationPromo | ||
| ? styles.portraitImage | ||
| : styles.image, | ||
| hasFixedAspectRatio | ||
| ? styles.imageFixedAspectRatio | ||
| : styles.imageResponsiveRatio, | ||
| isCurationPromo && | ||
| !isLoading && { | ||
| [`@supports (filter: blur(15px))`]: { | ||
| background: `rgba(${colour?.rgb?.join(',')}, 0.62)`, | ||
| }, | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Aaron on making a separate component for this as I'm not 100% sure about having these changes in our native image component for this "temporary" fix. It feels like the colour stuff should be encapsulated within it's own component to prevent any accidental re-rendering or other side effects. This feels more permanent than temporary. Is there something simpler we could do like just having a black background across the board? If we stick with the isPortrait mechanism, we should probably align the API we have already for the orientation value that we use on media players, to make it a lil more consistent. Maybe we do a huddle on this to discuss our options? |
||
| isCurationPromo && { | ||
| background: `rgb(${colour?.rgb?.join(',')})`, | ||
| }, | ||
emilysaffron marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ]} | ||
| fetchPriority={fetchPriority} | ||
| style={{ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.