chore(Video): Conditionally Render Youtube/Vimeo Videos #3049
Conversation
|
View your CI Pipeline Execution ↗ for commit f97d1f1. ☁️ Nx Cloud last updated this comment at |
smithgajjar09
changed the title
📬Published Alpha Packages:@codecademy/gamut@59.3.1-alpha.f97d1f.0 |
|
🚀 Styleguide deploy preview ready! |
| @@ -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/)); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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; | ||
| }; |
There was a problem hiding this comment.
instead of this being based on the hosting site, can we have it based on if captions are provided or not?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
ah gotcha! that makes sense, i just misread the logic here
Overview
Conditionally Render Youtube/Vimeo videos if captions provided or fallback to default embed
PR Checklist
Related to designs:Testing Instructions
Over here -> https://github.com/codecademy-engineering/mono/pull/9574
PR Links and Envs