Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DS-1146 DS-1193 | Looping video page heroes and inline Story Video component; reusable Caption, MediaWrapper and various Video components #389

Merged
merged 41 commits into from
Feb 28, 2025

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Feb 6, 2025

READY FOR REVIEW

Summary

  • Add background and foreground video looping to story page heroes
  • Add SbVideo - inline looping video component with caption and other options
  • Add Video, MutedVideoLoop, StoryVideo and VideoButton components
  • Create MediaWrapper and Caption components which replace part of the common code in StoryImage, StoryVideo and EmbedMedia components

Review By (Date)

  • Sooner the better

Criticality

  • 6

Review Tasks

Setup tasks and/or behavior to test

  1. Review pages in Slack conversation:
    https://stanfordwebservices.slack.com/archives/C03R765SAJC/p1739990416255369
    https://stanfordwebservices.slack.com/archives/C03R765SAJC/p1740163729849879
  2. Look at the deploy preview and make sure that the story image and embed media components still work the same as before:
    https://deploy-preview-389--giving-campaign.netlify.app/initiatives/stanford-public-humanities
    https://deploy-preview-389--giving-campaign.netlify.app/stories/team-behind-the-teams (The Annotated Image component uses StoryImage)
    https://deploy-preview-389--giving-campaign.netlify.app/stories/stretching-the-canvas
  3. Look at code and things called out

Associated Issues and/or People

@yvonnetangsu yvonnetangsu marked this pull request as draft February 6, 2025 01:34
Copy link

netlify bot commented Feb 6, 2025

Deploy Preview for giving-campaign ready!

Name Link
🔨 Latest commit 7d7bdfb
🔍 Latest deploy log https://app.netlify.com/sites/giving-campaign/deploys/67c20c2f56629b0008ac0e94
😎 Deploy Preview https://deploy-preview-389--giving-campaign.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

* dev:
  NOJIRA | Remove currently unused and POC components (#390)
…e the first frame of the video and should have the same default focus as the video itself; update getProcessedImage to resize image to 2000px wide by default if image is larger than 2000px and if crop dimension is not provided
@yvonnetangsu yvonnetangsu changed the title DS-1146 | Looping video heroes DS-1146 DS-1193 | Looping video page heroes and inline Story Video component Feb 21, 2025
@yvonnetangsu yvonnetangsu marked this pull request as ready for review February 27, 2025 21:33
@@ -16,7 +19,7 @@ import { getMaskedAsset } from './getMaskedAsset';

export const getProcessedImage = (
imageSrc: string = '',
crop: string = '',
crop: string = getSbImageSize(imageSrc)?.width > 2000 ? '2000x0' : '',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a small change to getProcessedImage utility, so that If a crop dimension is not provided and if the image width is greater than 2000px, the crop dimension will be set to "2000x0" to keep the original aspect ratio and downsize the image to 2000px wide. I don't think there is a reason to have any images on the site that is larger than 2000px wide at the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart.

Comment on lines +1 to +33
import { VideoHTMLAttributes, forwardRef } from 'react';
import { getMaskedAsset } from '@/utilities/getMaskedAsset';
import { getProcessedImage } from '@/utilities/getProcessedImage';

/**
* Simple Video component that allows for webm and mp4 sources and a poster image.
*/

export type VideoProps = VideoHTMLAttributes<HTMLVideoElement> & {
mp4Src?: string;
webmSrc?: string;
posterSrc?: string;
}

export const Video = forwardRef<HTMLVideoElement, VideoProps>(({
mp4Src,
webmSrc,
posterSrc,
children,
className,
...props
}, ref) => (
<video
ref={ref}
poster={getProcessedImage(posterSrc)}
className={className}
{...props}
>
{webmSrc && <source src={getMaskedAsset(webmSrc)} type="video/webm" />}
{mp4Src && <source src={getMaskedAsset(mp4Src)} type="video/mp4" />}
{children}
</video>
));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic html video component that supports a mp4 and webm sources (plus any additional via children prop), and a preview image poster attribute

Comment on lines +8 to +10
export const MutedVideoLoop = forwardRef<HTMLVideoElement, VideoProps>((props, ref) => (
<Video ref={ref} role="presentation" muted autoPlay loop playsInline {...props} />
));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A convenience component for muted loops so I don't have to write out those same attributes all the time.

aspectRatioClass?: string;
};

export const StoryVideo = ({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a counterpart of the StoryImage component - comes with a wrapper that provides aspect ratio, width and spacing options, plus caption and other options.

Comment on lines +17 to +21
export const Caption = ({
caption,
isCaptionInset,
captionBgColor = 'transparent',
className,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract the Caption component since it's used in a few places

aspectRatioClass?: string; // Additional eg, responsive aspect ratio classes
};

export const MediaWrapper = ({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrapper component that's used for StoryImage and StoryVideo that provides it with a common set of options

Comment on lines 71 to 84
/**
* Pause video when it goes out of view,
* resume when it comes back into view if it was not manually paused by the user.
*/
useEffect(() => {
const video = videoRef.current;
if (!video) return;

if (isVideoInView && !isUserPaused) {
video.play().catch(() => {});
} else {
video.pause();
}
}, [isVideoInView, isUserPaused]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pause the video when 90% or more is out of the viewport. Resumes playing when it comes back into view, unless user had previously manually paused the video.

@yvonnetangsu yvonnetangsu changed the title DS-1146 DS-1193 | Looping video page heroes and inline Story Video component DS-1146 DS-1193 | Looping video page heroes and inline Story Video component; reusable Caption, MediaWrapper and various Video components Feb 27, 2025
…ve behavior but if used in newer version of Framer Motion this matters) and update wrong doc
@@ -45,7 +45,7 @@ export const AnimateInView = ({
duration,
ease: 'easeOut',
}}
initial="hidden"
initial={beforeAnimationState}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated AnimateInView wrapper. This still works as intended on live, but when this component is used with a newer version of Framer Motion, it doesn't work correctly. This get rid of the conflict and should be this way from the beginning - I thought this looked sus at one point and always meant to come back to do it properly even though it was still working.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I found the bug was that Rebecca is using my AnimateInView component on Centennial which is on React 19/Next 15 and the latest version of Framer Motion (now called just Motion). She was wondering why the reduced motion setting didn't work for her site, but still works for Momentum. I came back to look at this and found that it's fixed if I make this change. @sherakama

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Good future gotcha prevention.


setIsBgPlaying((prev) => {
if (prev) {
bgVideoRef.current?.pause();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need the ? here as you've already checked if the .current value is truthy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I'll update 😄

} else {
bgVideoRef.current
?.play()
.catch(() => {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me more about this catch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this because sometimes video playback might be blocked or it can't be played for some reason, which can cause issues with DOM errors. This catches the error silently and prevents that from causing issue - I could have it display a console error or warning as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the user experience if the video playback fails and you've squashed the error? Can the user self-recover? Is there a graceful failover?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I can certainly do more with error handling, but error should be rare. So in the interest of time, I'll come back to this later @sherakama 😂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let you judge the time/effort to spend here but overall I am not a fan of squashing errors quietly as they tend to be very difficult to troubleshoot. I'd rather see the error logged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update for it to show a console warning 👍🏼

@sherakama
Copy link
Member

Happy path testing works (and looks) great.
You've done some good work on abstracting and consolidating. Looks like there is potential for some more as there is a little code duplication going on here between the image and video components but nothing that should be a blocker IMHO.

I added @ericbakenhus for visibility and his keen eye too.

@yvonnetangsu
Copy link
Member Author

yvonnetangsu commented Feb 28, 2025

Happy path testing works (and looks) great. You've done some good work on abstracting and consolidating. Looks like there is potential for some more as there is a little code duplication going on here between the image and video components but nothing that should be a blocker IMHO.

I added @ericbakenhus for visibility and his keen eye too.

Yes, I remember what you suggested before about a responsive image component 😄 I still plan on doing that at some point, but going to get this out the door first. It's a busy sprint 🤠 Some final clean up of redundant props coming (get rid of those that are already in MediaWrapperProps)

@yvonnetangsu
Copy link
Member Author

I'll have to find out more about this "happy path testing" @sherakama 👀

@sherakama
Copy link
Member

I'll have to find out more about this "happy path testing" @sherakama 👀

Well, not a long path on this one, but the testing was:

  1. Load page
  2. See video play
  3. Resize page
  4. Pause
  5. Reize page
  6. Play

@yvonnetangsu
Copy link
Member Author

I'll have to find out more about this "happy path testing" @sherakama 👀

Well, not a long path on this one, but the testing was:

  1. Load page
  2. See video play
  3. Resize page
  4. Pause
  5. Reize page
  6. Play

Cool 😎 Yeah I've never heard that term before so I just looked that up.

…in the shared MediaWrapperProps; better doc for aspectRatioClass; add console warning if video playback is causing error
@yvonnetangsu
Copy link
Member Author

Thank you for the review 😄 I have tested again after the final changes. Going to cut a release for this.

@yvonnetangsu yvonnetangsu merged commit a3ae013 into dev Feb 28, 2025
5 checks passed
@yvonnetangsu yvonnetangsu deleted the feature/DS-1146/video-heroes branch February 28, 2025 19:29
@yvonnetangsu yvonnetangsu mentioned this pull request Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants