Problem/Motivation
Basic features:
- mute
- autoplay
- loop
- show controls
- lazy loading: https://vidstack.io/docs/wc/player/core-concepts/loading/?styling=defaul...
- poster image
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
Issue fork vidstack_player-3508034
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
changes, plain diff MR !1
- revert-08a456be
compare
Comments
Comment #2
anybodyComment #3
anybodyComment #4
lrwebks commentedComment #6
lrwebks commentedThe 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.
Comment #7
lrwebks commentedComment #8
grevil commentedNice 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! 👍
Comment #9
grevil commented@lrwebks, was this ready for rereview again?
Comment #10
lrwebks commented@grevil, the README is still missing, so I'll take care of that and then put the issue to “needs review”.
Comment #11
lrwebks commentedComment #12
lrwebks commentedComment #13
lrwebks commentedA 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.
Comment #14
lrwebks commentedThis is off to review again, and as far as I can see, done (implementation-wise)!
Comment #15
anybodyFor final test and review by @thomas.frobieter! 🎉
Comment #16
grevil commentedComment #17
grevil commentedBack to "NW" based on my comment. Also check our internal vidstack issue on clickup for further comments by @thomas.frobieter.
Comment #18
grevil commented@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.
Comment #19
grevil commentedComment #20
lrwebks commentedComment #21
anybodyComment #22
grevil commented@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.
Comment #23
grevil commented@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.
Comment #24
grevil commentedYep that fixed the cache issue.
Comment #25
grevil commentedInteresting, 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.
Comment #26
grevil commentedOk, 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!!
Comment #28
grevil commented