Support embedded Youtube Atom in Card - #8075
Merged
Merged
Conversation
|
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
June 28, 2023 16:06
fe71555 to
17bb65c
Compare
mxdvl
marked this pull request as ready for review
June 28, 2023 16:13
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
June 28, 2023 16:21
17bb65c to
16b0ef7
Compare
This was referenced Jun 28, 2023
mxdvl
commented
Jun 28, 2023
mxdvl
left a comment
Contributor
Author
There was a problem hiding this comment.
Little code pointers below 👇
| @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 |
Contributor
Author
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, |
Contributor
Author
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
June 29, 2023 14:05
16b0ef7 to
b747b27
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
2 times, most recently
from
June 29, 2023 16:23
26309b2 to
cf02622
Compare
jamesgorrie
reviewed
Jun 30, 2023
jamesgorrie
left a comment
Contributor
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' && ( |
Contributor
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' && ( |
Contributor
Author
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.
Contributor
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.
Contributor
Author
jamesgorrie
reviewed
Jun 30, 2023
mxdvl
force-pushed
the
mxdvl/refactor-ad-targeting
branch
from
June 30, 2023 14:53
f2538d7 to
d36891e
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
3 times, most recently
from
June 30, 2023 16:21
838a3aa to
1257340
Compare
ioannakok
reviewed
Jul 3, 2023
| expired?: boolean; | ||
| activeVersion?: number; | ||
| /** @deprecated */ | ||
| channelId?: unknown; |
Contributor
There was a problem hiding this comment.
Why are defaultHtml and channelId unknown and not string?
Contributor
Author
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
July 3, 2023 09:03
6757511 to
b99c0c0
Compare
ioannakok
approved these changes
Jul 3, 2023
Contributor
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
| )} | ||
| {image.type === 'video' && ( | ||
| <div | ||
| data-chromatic="ignore" |
Contributor
Author
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
July 4, 2023 09:47
d96fdeb to
591a366
Compare
Contributor
Author
|
Waiting on #7972 before merging cc @abeddow91 |
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
July 4, 2023 09:58
591a366 to
30da4a6
Compare
abeddow91
force-pushed
the
ab/video-card-redesign
branch
from
July 4, 2023 14:57
ea61540 to
1a01a5b
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
July 4, 2023 15:18
30da4a6 to
3b08554
Compare
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
July 4, 2023 17:00
3b08554 to
df5ac31
Compare
mxdvl
commented
Jul 4, 2023
| avatarUrl?: string; | ||
| mediaType?: MediaType; | ||
| mediaDuration?: number; | ||
| showMainVideo: boolean; |
Contributor
Author
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
arelra
left a comment
Member
There was a problem hiding this comment.
A fine example of a collaborative PR!
mxdvl
force-pushed
the
mxdvl/video-atom-in-card
branch
from
July 5, 2023 14:09
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
July 5, 2023 15:28
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
July 5, 2023 15:37
9ddf882 to
14f65ea
Compare
mxdvl
commented
Jul 17, 2023
| mediaDuration: | ||
| faciaCard.properties.maybeContent?.elements.mediaAtoms[0] | ||
| ?.duration, | ||
| showMainVideo: faciaCard.properties.showMainVideo, |
mxdvl
commented
Jan 17, 2024
| /** For displaying embedded, playable videos directly in cards */ | ||
| type Video = Media & { | ||
| type: 'Video'; | ||
| elementId: string; |
Contributor
Author
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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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