-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
Size Change: +1.61 kB (+3%) Total Size: 51.3 kB
ℹ️ View Unchanged
|
Nice!
I prefer 2) mainly in order to: Which just leaves a UX question of if a generic placeholder is acceptable. Also a minor point is when I've done my own experiments loading in the player on a user click, it can cause a bit of jank when it loads in. I don't know if this is noticeable enough to be concerned about though. |
You can test this by running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really interesting approach. I’ve got a couple of questions:
- I wonder what happens on touch devices where there’s no such concept of hover.
- Don’t we want to autoplay videos sometimes? Maybe this will be addressed with a subsequent PR.
Otherwise a couple of minor points below 👇
width: 500, | ||
src: | ||
'https://i.guim.co.uk/img/media/4b3808707ec341629932a9d443ff5a812cf4df14/0_309_1800_1081/master/1800.jpg?width=1200&height=630&quality=85&auto=format&fit=crop&overlay-align=bottom%2Cleft&overlay-width=100p&overlay-base64=L2ltZy9zdGF0aWMvb3ZlcmxheXMvdGctZGVmYXVsdC5wbmc&enable=upscale&s=aff4b8255693eb449f13070df88e9cac', | ||
}, | ||
], | ||
}, | ||
]} | ||
height={450} | ||
width={800} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
450×800 isn’t the same ratio as 630×1200… 🤔
Maybe this is why you’ve experienced jank, @arelra?
In this case it will be the click (or touch) event that triggers the content download. We could also consider using an observer to do the same thing when a video scrolls into view? Most of our traffic is mobile so we should be favouring this use case.
We autoplay for video articles. These are not currently supported by DCR but if we wanted to do this here I think that's a simple prop. Obviously, if we do this then we lose all of these benefits. |
Co-authored-by: Max Duval <[email protected]>
…/atoms-rendering into oliver/you-tube-consent
Co-authored-by: Max Duval <[email protected]>
Based on @mxdvl 's comments I think a nice second round PR for this work would be an enhancement where we have two modes for how we decide when to load the iframe. One for mobile where we use scroll position, and the other for desktop where we use mouse position. To get us there, I would like to look at raising up some of the helper utilities that are currently in this PR. A suite of functions like |
Are we happy that this PR is okay to go through as it stands? @arelra @mxdvl @SiAdcock ? Considerations
|
…ns should be the consumers problem
I've tested this code in isolation (via Storybook) and when loaded into DCR and I'm seeing the expected behaviour in each case. Next steps:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 From me!
What does this change?
This PR affects the way we load the you tube code to play videos. Instead of eager loading the iframe we now wait until the reader has either clicked or hovered over the poster image.
In addition, we also wait for
consentState
to exist.Why?
This reduces the amount of content and javascript we need to download on page render by at least 200Kb, deferring that work until needed.
Why do we wait for consent?
We need consent to decide the ad targeting value that we pass to YouTube. By deferring the load of the iframe we can ensure that we only make this call when we have all the required information.
Placeholder
?There's an edge case where if a video does not have a poster image (rare) and we have not received the
consentState
back from CMP then we need to decide what to show. We could do one of two things:This PR has implemented point 2 but this is open to discussion
Showing YouTube loading on mouse over
Screen.Recording.2021-11-22.at.12.26.59.mov
Simulating consent being set late
Screen.Recording.2021-11-22.at.12.47.37.mov