Problem/Motivation

I am a happy user of Plyr but found this here on GitHub: https://github.com/sampotts/plyr/discussions/2871

It turns out that there is a new Video.js 10 which includes major improvements. While is still alpha we should see what is needed to get in support for the latest version of Video.js

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork videojs-3577815

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

yannickoo created an issue. See original summary.

semiaddict made their first commit to this issue’s fork.

semiaddict’s picture

semiaddict commented

Hi,
I need this fairly urgently.
I will thus work on an implementation for Video.js 10 in a fork and will submit an MR soon.

yannickoo’s picture

yannickoo
Location Berlin
commented

The Video.js team has updated their website and now you can already see the beta on https://videojs.org 😏

semiaddict’s picture

semiaddict commented

Yes. I saw that :)
I submitted a feature request to include dist files as GitHub release assets to be able to use them as library files in the drupal module.
I will use the CDN files for now.

yannickoo’s picture

yannickoo
Location Berlin
commented

Is this code from their website not enough?

<script type="module" src="https://cdn.jsdelivr.net/npm/@videojs/html/cdn/video.js"></script>
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@videojs/html/cdn/video.css" />
semiaddict’s picture

semiaddict commented

I just pushed a first commit to the 4.x branch that uses the CDN files.
The commit also adds support for audio files.

yannickoo’s picture

yannickoo
Location Berlin
commented

Thank you for your work @semiaddict, really nice base to continue working on the v10 implementation 💪

Since we cannot have a MR for your changes I need to comment here :D

    <{{ mediaTagName }} slot="media" preload="{{ player_attributes.preload }}" {{ player_attributes.autoplay ? 'autoplay' : '' }} {{ player_attributes.loop ? 'loop' : '' }} {{ player_attributes.muted ? 'muted' : '' }}>

Could you always add the playsinline attribute so mobile browsers can play media inline?

Another question re slot="media" – Why is that needed? The default example on Video.js website says this which does not need a slot attribute:

<video-player>
  <video-skin>
    <video src="https://stream.mux.com/BV3YZtogl89mg9VcNBhhnHm02Y34zI1nlMuMQfAbl3dM/highest.mp4" playsinline></video>
  </video-skin>
</video-player>

anybody opened merge request !19

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Why not create a Draft MR against 3.x and switch over to 4.x once it was created? I did that.

So we can better review and comment the code.

anybody opened merge request !20

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@yannickoo I think we should push things forward here. You've done a great job, would you maybe like to try contacting the current maintainers about their VideoJS 10 and 4.x plans and maybe offer (Co-)maintainership?
We as DROWL team would also like to help if the maintainers are not planning to push this forward. I'd like not to do a fork for that, but didn't see maintainer activity here yet.

@semiaddict I guess you're also interested?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Just worte @_shy a message asking for further plans:

Hi Kostia,

hope you're fine.

Just wanted to reach out and ask about your plans for VideoJS 10: https://www.drupal.org/project/videojs/issues/3577815
Are you still actively maintaining the module?

Would you mind commenting the issue and share your plans and maybe create a 4.x branch or 10.x branch (to keep it in sync with Video.js) to work on?

Thanks a lot!

Julian
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: Prepare 4.x branch for Video.js 10 implementation » Prepare 10.x branch for Video.js 10 implementation

I'd vote to have the branch in sync with videojs branch, so let's use 10.x

grevil’s picture

grevil commented
Title: Prepare 10.x branch for Video.js 10 implementation » [10.x] Prepare 10.x branch for Video.js 10 implementation
_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented

Hi guys, thanks for your contribution.

I created a new 4.x branch so we can work with that now. Moved the MR from the Draft state.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Hi @_shy, thanks! Would you accept to use a 10.x branch instead to keep it in sync with the library version this module is based on, for the future?

For library-focused / based modules that makes a lot of sense, because it makes it a lot easier to understand and maintain to keep things in sync. So if VideoJS 11 contains braking changes (due to SemVer) we can simply solve this in 11.x etc.
A really helpful pattern.

Maybe you can still delete 4.x then and make 10.x the default in GitLab?

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented

Hi @anybody!

Yes, I agree it makes sense for me. Let's use 10.x branch instead.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Version: 3.x-dev » 10.x-dev
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thank you so much @_shy! @grevil will start working here right now!

grevil opened merge request !21

grevil’s picture

grevil commented

Yes already on it. Created a seperate branch and used the work from @yannickoo and @semiaddict as a base.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil

thomas.frobieter made their first commit to this issue’s fork.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: grevil » Unassigned
Status: Active » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » anybody

Reviewing the current status, thanks @grevil!!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: anybody » grevil
Status: Needs review » Needs work

Nice work @grevil - left my comments! :)

As discussed we should maybe move the dialog parts into the dedicated follow-up. At least the XHTML part, which doesn't look complete anyway.

grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs work » Needs review

All green! I think this is ready to merge @anybody?

The rest of the features / follow-ups will be done in the other issues

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Thanks @grevil great work! Let's merge this and keep 10.x releases in an alpha state for quite a while!
Let's proceed in the follow-ups.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Fixed

@anyone reading this: Please help us testing and provide feedback!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • anybody committed 88c76e9c on 10.x authored by grevil 
    feat: #3577815 [10.x] Prepare 10.x branch for Video.js 10 implementation
grevil’s picture

grevil commented

Thanks @grevil great work! Let's merge this and keep 10.x releases in an alpha state

We shouldn't release anything for now until the remote formatter and poster works correctly.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.