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

fix: add native cues when using native text tracks #719

Closed
wants to merge 2 commits into from

Conversation

third774
Copy link
Contributor

@third774 third774 commented Jan 17, 2020

Co-authored-by: Kyle Boutette kyleveb@gmail.com

Description

When native text tracks are used, the cues added need to be native cues.

Related to this PR in the player: videojs/video.js#6410

Specific Changes proposed

Checks to see if native tracks are being used, and if so converts the cue into a native cue.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Jan 17, 2020

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@third774
Copy link
Contributor Author

Not sure how best to go about creating an example with this fix implemented. It's also dependent on the related PR in the player.

@third774
Copy link
Contributor Author

Additionally, not sure what adding tests for this would look like. With a little guidance I'd be happy to take a stab at it! 😄

@third774
Copy link
Contributor Author

third774 commented Jan 21, 2020

Here's a demo of native captions working with this change!

The video.js file in this demo was built with video.js on commit ff9fd1c which is the related PR videojs/video.js#6410 and having linked the @videojs/http-streaming dep to this PR's commit 86dfba9

https://third774.github.io/videojs-native-captions-fix/

@gkatsev
Copy link
Member

gkatsev commented Jan 27, 2020

Thanks @third774, we'll take a look soon.

@third774
Copy link
Contributor Author

third774 commented Feb 6, 2020

Looks like this breaks a few tests... looking into this now.

src/vtt-segment-loader.js Outdated Show resolved Hide resolved
Co-authored-by: Kyle Boutette <kyleveb@gmail.com>
@third774
Copy link
Contributor Author

Added a test for this as well!

@third774
Copy link
Contributor Author

Just wanted to circle back around on this — is there anything else blocking this change (and related PR videojs/video.js#6410) from being merged?

@gkatsev
Copy link
Member

gkatsev commented Mar 10, 2020

@third774 hey, thanks for your patience! We'll take a look in the next day or so.

@gkatsev
Copy link
Member

gkatsev commented Mar 11, 2020

Closing this in favor of #759, I rebased it and has a tiny change in the test. I'll merge it manually to maintain authorship credit once it's reviewed.

@gkatsev gkatsev closed this Mar 11, 2020
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.

3 participants