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

Video: Update to VidstackPlayer library #5609

Merged
merged 37 commits into from
Jul 23, 2024
Merged

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Jul 4, 2024

Closes #4914
Closes #5479

Updated Video to use VidstackPlayer library. The Plyr library was archived in favor of the new library sampotts/plyr#2737

In this version, I have made it work as closely as before, and from my tests, it should not have any breaking changes.

@stsrki stsrki changed the title WIP: Update Video to use VidstackPlayer Video: Update to VidstackPlayer library Jul 19, 2024
@stsrki stsrki requested a review from David-Moreira July 20, 2024 07:29
Copy link
Contributor

@David-Moreira David-Moreira left a comment

Choose a reason for hiding this comment

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

  • I can't tell if changing the quality does anything
  • Changing captions or speed does not seem to have any effect, I don't see any subtitles
  • Sometimes when clicking changing video, nothing happens, page seems broken.

@stsrki
Copy link
Collaborator Author

stsrki commented Jul 22, 2024

  • I can't tell if changing the quality does anything

This should not be changed, hence the DefaultQuality parameter name. I removed the change button.

  • Changing captions or speed does not seem to have any effect, I don't see any subtitles

Yeah, this seems to not work with Plyr layout while it works with default layout. I might report the issue on their repo.

  • Sometimes when clicking changing video, nothing happens, page seems broken.

For me it works. Cannot reproduce.

@stsrki stsrki requested a review from David-Moreira July 22, 2024 11:24
@stsrki
Copy link
Collaborator Author

stsrki commented Jul 22, 2024

The menu should work now.

@David-Moreira
Copy link
Contributor

DEMO:

  • Go To 20 seconds only works once. Maybe the CurrentTime needs to be bound?
  • It would be nice if clicking change video would alternate between both videos
  • Menu seems to work perfectly right now, but I still don't get any captions at all, even though the options are there? Is this intended?
  • When you click stop, you can only scrub the video, you can no longer play/pause the video, the video just stays paused?

DOCS:

  • Is a video missing here
    image
  • The Widevine DRM instantiation doesn't seem to play at all?
  • The Multiple files example, displays 3 files being bound, but you can't really use the other files? Would it not be great to see it, being able to select any of the 3 files?

@stsrki stsrki requested a review from David-Moreira July 23, 2024 05:01
@stsrki
Copy link
Collaborator Author

stsrki commented Jul 23, 2024

I have fixed the problems you mentioned. Some other things were already behaving as before, so I will skip them to avoid postponing the release.

Proceeding with the merge 🤞

@stsrki stsrki merged commit 6a8f2bd into master Jul 23, 2024
2 checks passed
@stsrki stsrki deleted the dev-video-update-to-vidstack branch July 23, 2024 09:06
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change video captions Update Video player to new library
2 participants