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

feat(Video): Adds Accessible Video Component #3015

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

smithgajjar09
Copy link
Contributor

@smithgajjar09 smithgajjar09 commented Jan 24, 2025

Overview

adds a new video component with Vidstack integration that is accessible with HLS support including captions, thumbnails, chapters and poster addon.

PR Checklist

  • Related to designs: Figma
  • Related to JIRA ticket: GM-985
  • I have run this code to verify it works
  • This PR includes unit tests for the code change
  • This PR includes testing instructions tests for the code change
  • The alpha package of this PR is passing end-to-end tests in all relevant Codecademy repositories

Testing Instructions

  • Check out the Video story + Full Example video/iframe section in Markdown
  • You should be able to watch the videos (with addon's and customisations like poster, thumbnails, captions)
  • Profit

PR Links and Envs

Repository PR Link PR Env
Monolith Monolith PR Monolith Env
Portal _ Portal Env
Mono Mono PR LE Env

Notes

  • Follow-up: GM-998 (Remove react-player)

Copy link

nx-cloud bot commented Jan 24, 2025

View your CI Pipeline Execution ↗ for commit db1e343.


☁️ Nx Cloud last updated this comment at 2025-02-20 18:03:33 UTC

@smithgajjar09 smithgajjar09 changed the title feat(Video): Vidstack VideoPlayer Integration feat(Video): A new accessible video component Feb 3, 2025
@smithgajjar09 smithgajjar09 changed the title feat(Video): A new accessible video component feat(Video): Adds Accessible Video Component Feb 3, 2025
@smithgajjar09 smithgajjar09 requested a review from a team as a code owner February 3, 2025 16:10
Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

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

looks good! just did a small perfunctory review, i'm going to dig into the css styles a little more tomorrow and test in mono.

Comment on lines 141 to 145
/**
* If showPlayerEmbed is true and Video Url is a string, use ReactPlayer to render the video
* Otherwise, use the Vidstack MediaPlayer. This is because currently vidstack player has an issue with
* youtube iframe embeds where it keeps pausing (only in case if yt iframe is used i.e with native yt controls)
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

is this recorded anywhere in Vidstack's backlog? most of our Codecademy content is from YT and i'm not a huge fan of keeping both media players/supporting both if we don't have to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a mention about this issue in Vidstack but not sure if there are actively looking into this issue yet. But after pooking around for a bit was able to add a workaround fix due to recent update in vidstack. We still have showPlayerEmbed flag to render ReactPlayer just as a fallback until we validate this player in the app. Adding a backlog ticket to remove

Copy link
Contributor

Choose a reason for hiding this comment

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

swee, that sounds good to me and we can talk to reed about the effort to move our Codecademy videos away from YT. can you link the backlog ticket # here and add to this epic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, here's the ticket - GM-998

Copy link
Member

@jakemhiller jakemhiller left a comment

Choose a reason for hiding this comment

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

looks good to me! I think it's worth getting @dreamwasp's final review before merging.

Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

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

this looks wonderful, great work!

Copy link
Contributor

@timjenkins timjenkins left a comment

Choose a reason for hiding this comment

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

Here's a half review (working on the 2nd half).
one main question: what's the use case for displaying react-player instead of vidstack?

Comment on lines 102 to 105
/**
* If showPlayerEmbed is true use ReactPlayer to render the video
* Otherwise, use the Vidstack MediaPlayer. @TEMPORARY_FALLBACK
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need both?

Copy link
Contributor Author

@smithgajjar09 smithgajjar09 Feb 20, 2025

Choose a reason for hiding this comment

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

This is more of just a fallback in case something goes sideways, and also when released all the youtube videos will be rendered using this new player (which does not have auto generated captions) So if curriculum asks to switch it to embed we have some way to make sure everything works
We have a follow up ticket to remove ReactPlayer once everything is validated - GM-998

@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/[email protected]
@codecademy/[email protected]
@codecademy/[email protected]
@codecademy/[email protected]

@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://67b76e8e1bcad50983e4431f--gamut-preview.netlify.app

Deploy Logs

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.

5 participants