Fixed Plyr container not resizing responsively

[shravan2x]

Link to related issue (if applicable): #1634

Summary of proposed changes

The main change was adding height: 100%s in a few places. I also had to update preview-thumbnails.js to support the new changes.

Checklist

• Use develop as the base branch
• Exclude the gulp build (/dist changes) from the PR
• Test on supported browsers

I use these fixes in a private app and they work fine. However, I had to change them a little when I copied them over. So this PR needs to be tested to verify that

• it fixes the problem and
• it doesn't inadvertently break something else.

I'd appreciate help testing it since I'm not able to do it myself at the moment.

[ghost]

just tested it and it works

[sampotts]

Thanks for looking into this and creating a decent PR! 👍

The only styling I'm concerned about is the height: 100% on the parent container. Is that needed?

Edit: Also, setting height on the video wrapper may break the ratio functionality. I'll do some testing though.

[shravan2x]

@sampotts Thanks for the quick response!

The height: 100% on both the parent and the wrapper seemed to be needed in my testing. Did you find otherwise?

I think it may also matter in which context users currently use Plyr. If they use it somewhere like a blog, the width: 100%; height: auto; might be a better choice (since blogs usually are vertically stacked). But for a full responsive experience, the height: 100% would work better IMO. If users want to constrain it to a specific size, they could just put the Plyr instance inside a container <div> of any size they want. We could retain backward compatibility by adding a responsive: boolean option in config, but I think most people would accept the new behavior as reasonable.

[ghost]

this is my testing. i use fancybox 3 with plyr in some of my pages and this is the results i get
before

after

[kgnfth]

@sampotts Why is height: 100%; removed from https://github.com/sampotts/plyr/blob/master/src/sass/base.scss

[sampotts]

It was this pull request:
#1997

I'm guessing it needs reverting then?

[kgnfth]

@sampotts im not the expert but after they removed it, its is back to this issues

[shravan2x]

When I initially made this PR, the height: 100% was definitely required. From #1997 it sounds to me like a Bootstrap issue or some missing code on their end.

[sampotts]

Could you try max-height: 100% on the .plyr element? Perhaps that's a happy medium.

[kgnfth]

@sampotts

but for vertical vids this happens

without max-height: 100%

with max-height: 100%

[kgnfth]

@sampotts it should be like this #1651 (comment)

[shravan2x]

@kgnfth Could you try adding max-height: 100% on both .plyr and .plyr-video-wrapper? You could also try adding an object-fit: contain on the latter.

[kgnfth]

@shravan2x same as above

you can try here https://tumbly.gocloud.fun/test/lolvideoslol

[shravan2x]

I think I have a solution that should fix both this issue and the other one. Working on another PR now.

[shravan2x]

@kgnfth Could you try shravan2x@c6376b6 #2022? If you aren't able to build Plyr entirely you could try making those changes in devtools directly. It's only a few fixes.

[sampotts]

Thanks for working this out and sorry for the hassle 👍

[shravan2x]

@sampotts Least I could do after all the work you've put into this amazing MIT licensed repository 🙂

If that fix works, the PR is #2022