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.

Sign up for GitHub

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

chore(Video): Conditionally Render Youtube/Vimeo Videos #3049

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+46 −9
Open

chore(Video): Conditionally Render Youtube/Vimeo Videos #3049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
609a083
chore: fallback yt and vimeo videos to older player
smithgajjar09 Mar 10, 2025
d047487
refactor: conditionally render video based on captions for yt and vimeo
smithgajjar09 Mar 11, 2025
eb7e671
Merge branch 'main' into sg-video-yt-fallback
smithgajjar09 Mar 11, 2025
2431e8a
Merge branch 'main' into sg-video-yt-fallback
smithgajjar09 Mar 24, 2025
0a48559
refactor: improve external video URL detection logic
smithgajjar09 Mar 24, 2025
f97d1f1
Merge branch 'sg-video-yt-fallback' of ssh://github.com/Codecademy/ga…
smithgajjar09 Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions packages/gamut/src/Video/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,13 @@ export type VideoProps = {
*/
translations?: Partial<DefaultLayoutTranslations>;
/**
* @TEMPORARY
* Determines if an embedded player view is shown.
* Determines if ReactPlayer is used to render youtube/vimeo videos.
* @default true
*/
showPlayerEmbed?: boolean;
/**
* Determines if the default provider/browser controls are shown.
* @default false
*/
showDefaultProviderControls?: boolean;
};
Expand Down Expand Up @@ -90,13 +91,43 @@ export const Video: React.FC<VideoProps> = (props) => {
},
};

const isExternallyHostedVideoUrl = (url: string): boolean =>
!!(url.match(/youtu(be\.com|\.be)/) || url.match(/vimeo.com/));
Copy link
Contributor

@LinKCoding LinKCoding Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

for consistency, it should be vimeo\.com

But also wondering if the com or be is necessary? like is there any instance where a different domain is used, like .co.uk
Or is this always ok because the URL is set by a dev and they'll conform to using these domains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really sure, this is something we already had in monorepo/author so just to make sure nothing breaks I copied them over here without any change. I would guess dev confirms the URL in author preview which checks for these regex('s)

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha, alrighties then I'd say that the period should still be escaped in both places
vimeo.com => vimeo\.com
but nothing crazy :)


const isExternallyHostedVideo = (videoUrl: PlayerSrc): boolean => {
if (!videoUrl) return false;

if (typeof videoUrl === 'string') {
return isExternallyHostedVideoUrl(videoUrl);
}

if (Array.isArray(videoUrl)) {
return videoUrl.some(
(url) => typeof url === 'string' && isExternallyHostedVideoUrl(url)
);
}

return false;
};
Comment on lines +97 to +111
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of this being based on the hosting site, can we have it based on if captions are provided or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have that check combined with this one -> Here (line 113-130)

So basically it checks if video source is youtube/vimeo and if captions are not provided we render ReactPlayer. This is because we also have few videos hosted on our S3 which gets rendered in HTML 5 player right now and this logic handles those videos to be able to render in new player -> Example

Do we want to switch all videos to ReactPlayer if captions are not provided ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah gotcha! that makes sense, i just misread the logic here


// TextTracks can also have chapters/descriptions/metadata, So we need to specifically check for subtitles/captions
const hasTracksWithCaptionOrSubtitle =
props.textTracks?.length &&
props.textTracks.some(
(track) => track.kind === 'subtitles' || track.kind === 'captions'
);

/**
* If showPlayerEmbed is true use ReactPlayer to render the video
* Otherwise, use the Vidstack MediaPlayer. @TEMPORARY_FALLBACK
* Render ReactPlayer if video is from youtube/vimeo and has no tracks with caption/subtitle.
* Otherwise, use the Vidstack MediaPlayer.
* @TODO [https://skillsoftdev.atlassian.net/browse/GM-998]
* Remove ReactPlayer once Vidstack is validated.
*/
if (showPlayerEmbed) {
if (
isExternallyHostedVideo(videoUrl) &&
!hasTracksWithCaptionOrSubtitle &&
showPlayerEmbed
) {
return (
<Box
position="relative"
Expand Down
7 changes: 5 additions & 2 deletions packages/gamut/src/Video/styles/vds_base_theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
@import "~@vidstack/react/player/styles/default/layouts/audio.css";
@import "~@codecademy/gamut-styles/utils";

// Youtube overlay
/**
* Youtube overlay
* - Why 120% ? hiding yt logo and copy link button which are not accessible.
*/
iframe.vds-youtube[data-no-controls] {
height: 100%;
height: 120%;
}

[data-started] iframe.vds-youtube[data-no-controls] {
Expand Down
4 changes: 4 additions & 0 deletions packages/styleguide/src/lib/Molecules/Video/Video.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ The Video component is a feature-rich video player that supports multiple video

## Variants

### HLS Stream With Captions, Chapters And Thumbnails

<Canvas of={VideoStories.VideoWithTracksAndThumbnails} />

### YouTube Video

<Canvas of={VideoStories.Youtube} />
Expand Down
3 changes: 1 addition & 2 deletions packages/styleguide/src/lib/Molecules/Video/Video.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const Vimeo: Story = {
},
};

/* export const VideoWithTracksAndThumbnails: Story = {
export const VideoWithTracksAndThumbnails: Story = {
args: {
videoUrl: 'https://files.vidstack.io/sprite-fight/hls/stream.m3u8',
videoTitle: 'Sprite Fight',
Expand Down Expand Up @@ -65,4 +65,3 @@ export const Vimeo: Story = {
thumbnails: 'https://files.vidstack.io/sprite-fight/thumbnails.vtt',
},
};
*/
Loading