Problem/Motivation

Basic features:

These setting should be available as field formatter settings for the video field.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

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:

  • 1.x Comparechanges, plain diff MR !1
  • 1 hidden branch
  • revert-08a456be Comparecompare

Comments

anybody created an issue. See original summary.

anybody’s picture

Issue summary: View changes
anybody’s picture

Issue summary: View changes
lrwebks’s picture

Status: Active » Needs work

lrwebks’s picture

The base functionality of the module is ready now and can already be reviewed, but I will still have to write a proper README.md next.

lrwebks’s picture

Status: Needs work » Needs review
grevil’s picture

Status: Needs review » Needs work

Nice work! I added a few comments. 🙂

I only looked at the code yet and haven't manually tested the module in our dev environment. So once everything is resolved, I can start doing that! 👍

grevil’s picture

@lrwebks, was this ready for rereview again?

lrwebks’s picture

@grevil, the README is still missing, so I'll take care of that and then put the issue to “needs review”.

lrwebks’s picture

Status: Needs work » Needs review
lrwebks’s picture

Status: Needs review » Needs work
lrwebks’s picture

A last thing that should be done is to take into account different image styles for the local video poster image, so I will do that next before we can call this one done.

lrwebks’s picture

Status: Needs work » Needs review

This is off to review again, and as far as I can see, done (implementation-wise)!

anybody’s picture

Assigned: lrwebks » thomas.frobieter

For final test and review by @thomas.frobieter! 🎉

grevil’s picture

Status: Needs review » Needs work
grevil’s picture

Back to "NW" based on my comment. Also check our internal vidstack issue on clickup for further comments by @thomas.frobieter.

grevil’s picture

@lrwebks can you remember why we didn't add a local library endpoint? If we wanted to do it in a follow-up issue, we should create that, but it could also be done here.

grevil’s picture

Assigned: thomas.frobieter » lrwebks
lrwebks’s picture

Status: Needs work » Needs review
anybody’s picture

Assigned: lrwebks » thomas.frobieter
grevil’s picture

@lrwebks the switch case adjustments wasn't fully, what needed to be done. We still had the case, where the poster setting is a valid field, BUT the fields target_id results in being NULL. Fixed that here: https://git.drupalcode.org/project/vidstack_player/-/merge_requests/1/di...

I also reverted the if case logic, so it is a bit more simple to read and we don't have deeply nested if cases.

grevil’s picture

Assigned: thomas.frobieter » Unassigned
Status: Needs review » Needs work

@lrwebks you forgot to use once, that's why the error appear. Also, we don't actually need to load the external library asynchronous. This probably led to the cache bug.

grevil’s picture

Yep that fixed the cache issue.

grevil’s picture

Interesting, if we create an image field on the media entity and let it be displayed. And then use the "rendered entity" display for our media reference field (on e.g. articles) we get the "Uncaught Vidstack Player JS library could not be loaded." error.

I'll create a follow-up issue for that. Not that important as Video Medias usually only display the video anyways.

grevil’s picture

Status: Needs work » Reviewed & tested by the community

Ok, we still get occasional issues, that "VidstackPlayer" is not defined, when changing settings. A simple cache clear fixes the issue, but of course this isn't perfect.

I'd say we merge the current status, as this could relate to the cdn using cdn calls internally or something else.

Great work @lrwebks!!

  • grevil committed 1799dc93 on 1.x authored by lrwebks
    Issue #3508034 by lrwebks, grevil: Implement basic features for...
grevil’s picture

Status: Reviewed & tested by the community » Fixed

grevil changed the visibility of the branch revert-08a456be to hidden.

Status: Fixed » Closed (fixed)

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