WS-1674 Implement Temporary UX Solution for Portrait Video Promo Display on Index Pages

[emilysaffron]

Resolves JIRA: WS-1674

Paper Doc

Summary

Detects 16x9 Portrait images and styles them to fit the 9x16 image promo to ensure the whole image is visible, and adds a gradient that pulls through a main colour from the image.

The detection of portrait images happens onLoad for lazy loaded images, and in a useEffect to account for images above the fold and/or that have been previously cached.

Code changes

• Detects portrait images onLoad and in a useEffect
• Uses existing useImageColour function to pull through the main colour of the image
• Changes image style to cover:contain to ensure whole portrait image is visible
• Adds div with gradient styling making use of the colour from the useImageColourFunction
• Updates snapshots and tests
• Increases bundle size due to the serbian topic page being too large

Testing

• Visit http://localhost:7080/persian/topics/c6z7mnr559gt?renderer_env=live and check it against the design idea laid out in the Paper Doc
• Doube check this solution doesn't break other places portrait video appears, eg PV carousel here and a media article containing a PV here

Additional Testing Steps

1. List the steps required to test this PR.

Useful Links

• Coding Standards
• Repository use guidelines
• Add Links to useful resources related to this PR if applicable.

[amoore108]

Just curious on the No JS fallback for this and how it looks?

[amoore108]

Looks like the changes have affected certain scenarios on the homepage:

[emilysaffron]

Looks like the changes have affected certain scenarios on the homepage:

~~Which asset/component is this, is the second image the one that's wrong? @amoore108~~

Ah figured it out - it's actually effecting a fair few things, fixing those this morning!

[emilysaffron]

Just curious on the No JS fallback for this and how it looks?

Good question - the no js makes the background colour default to black, but because we assume that all images are portrait by default they still look ok . The landscape images will also broadly look okay, apart from those occasional ones that dont fill out their container - i think this is acceptable?

[emilysaffron]

Looks good - I'd be interested to know if this has an impact on web vitals (more because I don't know if applying gradient would affect CLS or anything).

I'm wondering if this could go onto preview to help with regression testing (i.e. clicking around and making sure no other promos are unintentionally affected).

Also it doesn't appear to work for me on Safari (there is just a black background). But I assume this is ok for our user base.

Same re web vitals! It feels like it might 😢 Can pop it onto preview, although i have had a check around on storybook and all seems okay. FWIW chatting with laura the requirement is for it to apply to all promos not just the ones on the index page, but probably worth double checking other pages?

[emilysaffron]

Looks good - I'd be interested to know if this has an impact on web vitals (more because I don't know if applying gradient would affect CLS or anything).

I'm wondering if this could go onto preview to help with regression testing (i.e. clicking around and making sure no other promos are unintentionally affected).

Also it doesn't appear to work for me on Safari (there is just a black background). But I assume this is ok for our user base.

Compared https://simorgh1.belfrage-preview.test.api.bbc.com/persian/topics/c6z7mnr559gt?renderer_env=live to https://www.bbc.com/persian/topics/c6z7mnr559gt and the CLS scores are identical 🥳

[amoore108]

Also it doesn't appear to work for me on Safari (there is just a black background). But I assume this is ok for our user base.

Tested on Safari mobile as well and it doesn't seem to work. Feel like this would be an issue as iOS is quite high usage for us.

[emilysaffron]

Also it doesn't appear to work for me on Safari (there is just a black background). But I assume this is ok for our user base.

Tested on Safari mobile as well and it doesn't seem to work. Feel like this would be an issue as iOS is quite high usage for us.

Strange - what version are you on? As it works for me on both safari mobile and desktop, checking https://simorgh1.belfrage-preview.test.api.bbc.com/persian/topics/c6z7mnr559gt?renderer_env=live @Isabella-Mitchell @amoore108

[Isabella-Mitchell]

Also it doesn't appear to work for me on Safari (there is just a black background). But I assume this is ok for our user base.

Tested on Safari mobile as well and it doesn't seem to work. Feel like this would be an issue as iOS is quite high usage for us.

Strange - what version are you on? As it works for me on both safari mobile and desktop, checking...>

Preview works for me on Safari desktop and mobile with the preview link. I had only tested local.

[amoore108]

Preview works for me on Safari desktop and mobile with the preview link. I had only tested local.

Thanks for the preview link, yea that works on desktop and mobile for me although quite visibly 'jumps' between the image filling the container and going portrait.

[emilysaffron]

Preview works for me on Safari desktop and mobile with the preview link. I had only tested local.

Thanks for the preview link, yea that works on desktop and mobile for me although quite visibly 'jumps' between the image filling the container and going portrait.

Yeahh i think i might check this with UX before merge , my decision process for this was here. The only way to prevent this really is to assume all images are portrait at first, but then this makes landscape images do the visible jumps, and it felt like those are more prominent across the site so we should prioritise those?

[amoore108]

Preview works for me on Safari desktop and mobile with the preview link. I had only tested local.

Thanks for the preview link, yea that works on desktop and mobile for me although quite visibly 'jumps' between the image filling the container and going portrait.

Yeahh i think i might check this with UX before merge , my decision process for this was here. The only way to prevent this really is to assume all images are portrait at first, but then this makes landscape images do the visible jumps, and it felt like those are more prominent across the site so we should prioritise those?

Ideally we should avoid the jumping behaviour completely, its happening because of the useEffect re-rendering when it figures out the orientation. Its a shame there isn't a pure CSS way here. Are the background colour gradients necessary at this stage? I assumed we'd just have a quick interim fix for this problem, but this feels more permanent.

[emilysaffron]

Preview works for me on Safari desktop and mobile with the preview link. I had only tested local.

Thanks for the preview link, yea that works on desktop and mobile for me although quite visibly 'jumps' between the image filling the container and going portrait.

Yeahh i think i might check this with UX before merge , my decision process for this was here. The only way to prevent this really is to assume all images are portrait at first, but then this makes landscape images do the visible jumps, and it felt like those are more prominent across the site so we should prioritise those?

Ideally we should avoid the jumping behaviour completely, its happening because of the useEffect re-rendering when it figures out the orientation. Its a shame there isn't a pure CSS way here. Are the background colour gradients necessary at this stage? I assumed we'd just have a quick interim fix for this problem, but this feels more permanent.

Yeahhh i know, ive just tried to play around with setting isPortrait to null iniitally and displaying the placeholder for longer while we figure out if the image is portrait or not, but i couldnt really get it working properly. I also have tried messing around with aspect ratios to see if i can get the pure css working but it doesnt seem possible to account for portrait and landscape this way 😢 I could check if the background colour is necessary, but even without it we'd still have the jumpy problem :/ @amoore108

[amoore108]

I can still see some snapshots in Chromatic with diffs that I wouldn't expect.

The other way around this would be to extract the gradient background into its own component and use it directly in the CurationPromo so that it only runs and effects there. This is the way FrostedGlassPromo does it: https://github.com/bbc/simorgh/blob/aae34f9201b02783bed4480d25b7b1c0c3c6ae60/src/app/components/FrostedGlassPromo/index.tsx#L138-L144

It has the Image as a distinct element, with the gradient calcuation in its own component so its isolated. You could create a PromoGlassPanel and use it in CurationPromo so it doesn't affect other components and keeps the Image component more pure. The FrostedGlassPromo wraps the panel part in Lazyload too, so its only loaded when in the viewport, which is useful on mobile.

Its good work here, feels more permanent than temporary though which is why I'm trying to keep the complexity and potential side-effects down. Just don't want a bunch of work to go into a solution that will be taken away? May be worth a huddle with a few folks to discuss?

[pvaliani]

Hey hey, I know this is temporary solution but just to highlight (if I'm understanding correctly) useImageColour now runs for every Image instance before checking isPromo/isPortraitImage according to the Image component?

I think that will trigger an extra image load and ColorThief run around here: https://github.com/bbc/simorgh/blob/330a991ecde214487aca3f1c34e8bcf4ff4de285/src/app/hooks/useImageColour/index.js#L31.

So there might be a risk that each canonical image (including any lazy loaded/offscreen renders) could start fetching/processing right away, bypassing lazy-load deferral for that extra fetch and that could add significant CPU/network cost across the site. Awesome job, this seems a bit fiddly

[emilysaffron]

I can still see some snapshots in Chromatic with diffs that I wouldn't expect.

The other way around this would be to extract the gradient background into its own component and use it directly in the CurationPromo so that it only runs and effects there. This is the way FrostedGlassPromo does it:

https://github.com/bbc/simorgh/blob/aae34f9201b02783bed4480d25b7b1c0c3c6ae60/src/app/components/FrostedGlassPromo/index.tsx#L138-L144

It has the Image as a distinct element, with the gradient calcuation in its own component so its isolated. You could create a PromoGlassPanel and use it in CurationPromo so it doesn't affect other components and keeps the Image component more pure. The FrostedGlassPromo wraps the panel part in Lazyload too, so its only loaded when in the viewport, which is useful on mobile.

Its good work here, feels more permanent than temporary though which is why I'm trying to keep the complexity and potential side-effects down. Just don't want a bunch of work to go into a solution that will be taken away? May be worth a huddle with a few folks to discuss?

Which ones in the chromatic would you not expect? Ive had a quick look through and it looks like it's all just promos that have changed which would be correct - i thought by keeping the isPromo boolean in place it would prevent it making changes to images everywhere - might have missed something though!

[amoore108]

Which ones in the chromatic would you not expect? Ive had a quick look through and it looks like it's all just promos that have changed which would be correct - i thought by keeping the isPromo boolean in place it would prevent it making changes to images everywhere - might have missed something though!

Theres a promo on the homepage in the billboard that shouldn't have a background:

Similarly in articles, I wouldn't expect these related content items to have a coloured background:

Editorial do use transparent background images on occasion, so I'm a bit concerned that this logic would always add some kind of background unexpectedly.

[emilysaffron]

Which ones in the chromatic would you not expect? Ive had a quick look through and it looks like it's all just promos that have changed which would be correct - i thought by keeping the isPromo boolean in place it would prevent it making changes to images everywhere - might have missed something though!

Theres a promo on the homepage in the billboard that shouldn't have a background:

Similarly in articles, I wouldn't expect these related content items to have a coloured background:

Editorial do use transparent background images on occasion, so I'm a bit concerned that this logic would always add some kind of background unexpectedly.

think id just missed a conditional off 🤦 , ive added it in now so this shouldnt happen anymore for non portrait, non promo images! checked these two locally and they were back to normal but will double check the chromatic when it's done

[emilysaffron]

Hey hey, I know this is temporary solution but just to highlight (if I'm understanding correctly) useImageColour now runs for every Image instance before checking isPromo/isPortraitImage according to the Image component?

I think that will trigger an extra image load and ColorThief run around here:

https://github.com/bbc/simorgh/blob/330a991ecde214487aca3f1c34e8bcf4ff4de285/src/app/hooks/useImageColour/index.js#L31

. So there might be a risk that each canonical image (including any lazy loaded/offscreen renders) could start fetching/processing right away, bypassing lazy-load deferral for that extra fetch and that could add significant CPU/network cost across the site. Awesome job, this seems a bit fiddly

~This is a good spot, i think i might have to check whether having the colour pull through is completely necessary with UX, because i think the only way around this would be to have this hook called in another component like the FrostedGlassPanel, and conditionally render that. This feels a bit much for a supposedly temporary solution, when we could just stick with the black or white background without having to call a hook at all?~

nvm i think i've prevented this a little by calling it conditionally just for portrait image promos