[SR] - [MWPW-193259] - Rich Content Video variant#5973
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
robert-bogos
left a comment
robert-bogos
left a comment
There was a problem hiding this comment.
I see you have some conflicts, might want to take a look
DKos95
requested review from
narcis-radu,
overmyheadandbody,
rgclayton,
robert-bogos and
zagi25
| margin: 0; | ||
| } | ||
|
|
||
| .video-container { |
There was a problem hiding this comment.
on mobile things seem a bit off, feels like someone would need to put a lot of effort to click on the CTA.
There was a problem hiding this comment.
Not really sure what can be done about this, the animation is that the bottom part gets pulled up, we can only slow it down if needed so the user can more easily scroll up in order for the bottom part to go down but I don't think design would like that.
There was a problem hiding this comment.
Will bring this up in a meeting today I have with Jordan.
There was a problem hiding this comment.
@DKos95 - If there's feedback from Design, maybe we can tackle it as a new requirement and have a separate JIRA ticket. @overmyheadandbody , @SilviuLCF
overmyheadandbody
left a comment
overmyheadandbody
left a comment
There was a problem hiding this comment.
I agree with Narcis that it seems like we're making it difficult for the user to click the CTA button, regardless if viewport. Design should be contacted to check this and/or find some alternatives.
On the performance side, we would likely want to pause the video when it's not in view, so there's no needless CPU load when other sections of the page are in view. Is this a planned update?
Otherwise this looks pretty nifty! Just a few more details to iron out.
| .rich-content.video { | ||
| .media { | ||
| padding-inline: unset; | ||
| max-width: var(--grid-width-10); |
There was a problem hiding this comment.
Keep in mind --grid-width-10 is only available inside a .container. That should usually be the case, but there is potential for an edge case here.
There was a problem hiding this comment.
We are using a container in this variant so we should be fine, I mean if someone takes out the container class the block will break(not look like it should) regardless, right?
I don't think this is requested in the ticket anywhere or at least I didn't see it. Also I am not sure how doable this is if the block has a scroll animation since it has a translate slow down so IntersectionObserve reading will be skewed a bit. I've tried to implement it but seems that the animation transform on the block is an issue I haven't found a workaround to pause the video when it visually goes off screen. |
|
@DKos95 - likely not requested officially, but as we've seen for the Wave 1 performance investigations, playing videos when they're off screen can needlessly load CPU, causing lag in other areas of the page. Router marquee implemented a solution; I also remember that the video logic used to have this OOTB in C1, I'm not sure what might've changed. Not a blocker right now, but likely a follow-up to score some performance wins. |
Like I said we have an animation on this block which is transform and it skews what the observer reads, which the router marquee doesn't have so the difference is probably there. I do agree that we should re visit it when we have time. |
narcis-radu
left a comment
narcis-radu
left a comment
There was a problem hiding this comment.
Looks good, but we will need additional effort to handle:
- CTA positioning - requires design input, we can hopefully treat it separately (new JIRA ticket - @SilviuLCF)
- performance - video should pause when it's out of view; we need some research here (new JIRA ticket @SilviuLCF)
|
Ticket for performance https://jira.corp.adobe.com/browse/MWPW-196476 |
SilviuLCF
left a comment
SilviuLCF
left a comment
There was a problem hiding this comment.
QA verified testing details https://jira.corp.adobe.com/browse/MWPW-193259
With follow up concerns
Ticket for performance https://jira.corp.adobe.com/browse/MWPW-196476
Ticket for CTA positioning https://jira.corp.adobe.com/browse/MWPW-196479
6 participants
PR adds video variant to rich-content block.
Resolves: MWPW-193259
Test URLs: