Support embedded Youtube Atom in Card #8075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -22.7 kB (-4%) Total Size: 622 kB
ℹ️ View Unchanged
|
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
2 times, most recently
from
fe71555 to
17bb65c
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
17bb65c to
16b0ef7
Compare
This was referenced Jun 28, 2023
mxdvl
commented
Jun 28, 2023
| @@ -200,6 +200,7 @@ gen-schema: | |||
| @git add src/model/article-schema.json | |||
| @git add src/model/front-schema.json | |||
| @git add src/model/block-schema.json | |||
| @git add src/model/tag-front-schema.json | |||
There was a problem hiding this comment.
Small refactors since #7752 introduced the new schema
Comment on lines
198
to
212
| // Size fixed to a 5:3 ratio | ||
| width: 500, | ||
| height: 300, |
There was a problem hiding this comment.
Cards do not have a concept of an intrinsic size, and calculating the size of the poster image seemed brittle.
arelra
reviewed
Jun 28, 2023
13 tasks
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
16b0ef7 to
b747b27
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
2 times, most recently
from
26309b2 to
cf02622
Compare
jamesgorrie
reviewed
Jun 30, 2023
There was a problem hiding this comment.
This PR looks great! Whilst it makes sense I got a bit confused.
Having two types for CardYoutubeVideo seemed a little leaky, especially as the transformation for either type is at different stages of the rendering pipeline (enhance vs cardWrappers.onlyKeepPlayIcon).
I wonder if we could make it more explicit of what we're doing with fewer transformations and follow some existing naming?
Here's a little pseudo code as I think I've been a little muddy in my explanation:
type DCRCardType = {
// ...other props...
// assuming this flattening is done as we prefer flat
showMainVideo: FECardType.properties.showMainVideo;
// video - this is identical to the image prop
video = decideVideo(FECardType);
}
type FrontCard = {
// ...other props...
showVideo: DCRCardType.showMainVideo;
video: DCRCardType.video;
// Maybe this isn't size, but follows the same pattern as images
// This gets in `cardWrappers` as that's what is aware of this context
// it is existing also an explicit property, rather than a transformation to an existing property
// it also doesn't leak the `show-only-icon` to `CardYoutubeVideo`
videoSize: "unplayable" : "playable";
}
// Card.tsx
const Card = ({ showVideo, video, videoSize }) => {
if (video && showVideo) {
if (videoSize === "playable") <VideoPlayer />
if (videoSize === "unplayable") <PosterWithIcon />
} else {
// image logic
}
}
Comment on lines
478
to
479
| {image.type === 'mainMedia' && | ||
| mainVideo?.type !== 'embedded-playable-video' && ( |
There was a problem hiding this comment.
I'd be tempted to swap this around to make the logic more analogous to:
if (isPlayableVideo) {
<Youtube />
} else if (image = isMainMedia) {
<CardPicture />
}Not sure if it's clearer just took my a couple seconds to work out what was going on.
Suggested change
| {image.type === 'mainMedia' && | |
| mainVideo?.type !== 'embedded-playable-video' && ( | |
| {mainVideo?.type !== 'embedded-playable-video' && | |
| image.type === 'mainMedia' && ( |
There was a problem hiding this comment.
I was wondering that too. Could not settle on a specific one so I’ll use your suggestion.
There was a problem hiding this comment.
My reasoning is that they aren't different types, it is rather a condition set by the parent component which has extra context.
jamesgorrie
reviewed
Jun 30, 2023
mxdvl
force-pushed
the
mxdvl/refactor-ad-targeting
branch
from
f2538d7 to
d36891e
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
3 times, most recently
from
838a3aa to
1257340
Compare
ioannakok
reviewed
Jul 3, 2023
dotcom-rendering/src/types/front.ts
Outdated
| expired?: boolean; | ||
| activeVersion?: number; | ||
| /** @deprecated */ | ||
| channelId?: unknown; |
There was a problem hiding this comment.
Why are defaultHtml and channelId unknown and not string?
There was a problem hiding this comment.
Based on a similar question by @jamesgorrie I decided to remove them, but somehow forgot.
My thinking was that we knew these fields come through, but we are not currently using them in any meaningful way. I wanted to make it clear to future travellers. I’ll just comment them out for now.
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
6757511 to
b99c0c0
Compare
ioannakok
approved these changes
Jul 3, 2023
There was a problem hiding this comment.
Great work! You have a +1 from me, don't know if it's best to also get @jamesgorrie's approval as he made all these great suggestions. My only doubt is:
export type VideoSize = 'playable' | 'unplayable';If we could name this something more relevant to the values it gets, that would be preferable but it's not a blocker.
mxdvl
commented
Jul 3, 2023
| alt={byline ?? ''} | ||
| containerPalette={containerPalette} | ||
| format={format} | ||
| /> | ||
| </AvatarContainer> | ||
| )} | ||
| {image.type === 'video' && ( | ||
| <div | ||
| data-chromatic="ignore" |
There was a problem hiding this comment.
Chromatic will ignore the content of this element – ping @cemms1.
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
2 times, most recently
from
d96fdeb to
591a366
Compare
|
Waiting on #7972 before merging cc @abeddow91 |
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
591a366 to
30da4a6
Compare
abeddow91
force-pushed
the
ab/video-card-redesign
branch
from
ea61540 to
1a01a5b
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
30da4a6 to
3b08554
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
3b08554 to
df5ac31
Compare
mxdvl
commented
Jul 4, 2023
| @@ -289,9 +309,7 @@ export type DCRFrontCard = { | |||
| byline?: string; | |||
| showByline?: boolean; | |||
| avatarUrl?: string; | |||
| mediaType?: MediaType; | |||
| mediaDuration?: number; | |||
| showMainVideo: boolean; | |||
There was a problem hiding this comment.
I could not find examples of when we want to hide the main video, so have removed as a simplication step.
arelra
approved these changes
Jul 5, 2023
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
df5ac31 to
ba8703a
Compare
renamed the method as it can handle more than images we can narrow our types with a discriminated union
an empty array should result in no poster image available.
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
ba8703a to
9ddf882
Compare
The settings comes from the facia-tool via Frontend, when “Show Video” is enabled for a trail in a container, it should be displayed as a playable video inline instead or redirecting to the article itself. Currently, only Youtube Atoms are supported. There is currently no guarantee that the first atom will be the expected video, but it happens to be the case the vast majority of the time A minimum Card width of one third or three columns is required for the YouTube atom to be embedded, similar to the existing Frontend logic: https://github.com/guardian/frontend/blob/1c4555a9f/common/app/layout/cards/CardType.scala#L20-L23
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
9ddf882 to
14f65ea
Compare
mxdvl
commented
Jul 17, 2023
| mediaDuration: | ||
| faciaCard.properties.maybeContent?.elements.mediaAtoms[0] | ||
| ?.duration, | ||
| showMainVideo: faciaCard.properties.showMainVideo, |
There was a problem hiding this comment.
Incorrectly removing this caused #8227
mxdvl
commented
Jan 17, 2024
| /** For displaying embedded, playable videos directly in cards */ | ||
| type Video = Media & { | ||
| type: 'Video'; | ||
| elementId: string; |
There was a problem hiding this comment.
This should not have been called elementId but simply id. It’s not the randomly generated frontend UUID, but rather a consistent value that uniquely identifies the main media atom.
Fix in #10218
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Enables support for
showVideofrom facia-tool, which allows videos to be played inline/embedded directly on fronts. Previously, it would redirect to the article itself.A minimum Card width of one third or three columns is required for the YouTube atom to be embedded, similar to the existing Frontend logic.
Why?
Closes #6662
Known issues
There is currently no guarantee that the first atom will be the expected video, but it happens to be the case the vast majority of the time. See guardian/frontend#26247 for a possible fix.
Currently, only Youtube Atoms are supported.
Screenshots