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

Updating to v3.5.7 breaks the player, for example at https://tobireif.com/posts/layout_fun_with_css_grid/ #1680

Closed
tobireif opened this issue Feb 10, 2020 · 12 comments

Comments

@tobireif
Copy link

The player at https://tobireif.com/posts/layout_fun_with_css_grid/ gets destroyed by the update.

Before:

screenshot_before

With v3.5.7:

screenshot_after

It would be really great if you could ensure that updates don't break things 😀

The release notes contain this text: "This update contains CSS changes."
I'd rather be able to update Plyr without having to dig through all the CSS changes and adapt my CSS. (For the linked page I have 87 lines of customization CSS for the Plyr player.)

@sampotts
Copy link
Owner

sampotts commented Feb 10, 2020

It would be really great if you could ensure that updates don't break things 😀

It would also be really great if users could look at the pull requests to see the changes.
https://github.com/sampotts/plyr/pull/1679/files?file-filters%5B%5D=.scss

I'd rather be able to update Plyr without having to dig through all the CSS changes and adapt my CSS. (For the linked page I have 87 lines of customization CSS for the Plyr player.)

I appreciate this might not be ideal but how do you suggest I make CSS changes without causing this in future? I'm all ears.

The options are:

CSS-in-JS

Something tiny like goober.

Pros:
✅ The CSS is bundled with the JS and you don't need to worry about it
✅ Colors can be customized a lot easier (something people have asked for a lot)

Cons:
❌ Potentially larger bundle size
❌ More advanced customisation would be difficult
❌ Funky classnames

Major versions

Every CSS update requires a major version update - i.e. v3.x.x to v4.x.x for any CSS change.

Pros:
✅ ?

Cons:
❌ Not really a major change in most cases
❌ Larger barrier to entry to upgrading
❌ Still not obvious what the changes are

@sampotts sampotts changed the title Updating to v3.5.7 breaks the player Updating to v3.5.7 breaks custom CSS Feb 10, 2020
@sampotts sampotts changed the title Updating to v3.5.7 breaks custom CSS v3.5.7 contains CSS changes Feb 10, 2020
@tobireif tobireif changed the title v3.5.7 contains CSS changes Updating to v3.5.7 breaks the player Feb 10, 2020
@tobireif
Copy link
Author

Perhaps CSS changes (if any are necessary) could be done in none-breaking ways?

@sampotts sampotts changed the title Updating to v3.5.7 breaks the player v3.5.7 contains CSS changes Feb 10, 2020
@sampotts
Copy link
Owner

In most cases they are but it's not really possible for me to know every location where the player is used and therefore what external CSS is going to be impacted.

@tobireif
Copy link
Author

Regarding the title: The issue is not that "v3.5.7 contains CSS changes" - the issue is that it contains breaking changes. Please leave the original title, or please ensure that the title reflects the issue that I reported.

@sampotts
Copy link
Owner

It doesn't "break the player" though in the majority of cases. It breaks it in your case. The title was misleading which is why I changed it.

@tobireif
Copy link
Author

The release still contains breaking CSS - please re-open the ticket.

@sampotts
Copy link
Owner

What do you want the issue to achieve? It just seems like a rant.

@tobireif tobireif changed the title v3.5.7 contains CSS changes Updating to v3.5.7 breaks the player, for example at https://tobireif.com/posts/layout_fun_with_css_grid/ Feb 10, 2020
@sampotts
Copy link
Owner

While the title tennis is fun, I have better things to do. Your efforts are better spent changing a couple of lines of CSS and moving on.

@tobireif
Copy link
Author

What do you want the issue to achieve?

Plyr downloads the poster image file several times #1531 instead of once. In order to update to a version which downloads the file twice instead of thrice I was asked to update the v3.5.7 #1531 (comment) . When I did that, my player was destroyed by breaking CSS in that release. It would be great if both of the issues could get resolved.

It just seems like a rant.

It it 's more than evident that it sounds like that to you, but it's merely a report describing a technical issue. Complete with a link to an example and with screenshots.

@sampotts
Copy link
Owner

Plyr downloads the poster image file several times #1531 instead of once. In order to update to a version which downloads the file twice instead of thrice I was asked to update the v3.5.7 #1531 (comment) . When I did that, my player was destroyed by breaking CSS in that release. It would be great if both of the issues could get resolved.

That's a different issue. I've just replied on that issue and I can't replicate it on the demo site. If others are seeing it multiple times then I'll take a look again or perhaps someone can take a look and open a PR.

It it 's more than evident that it sounds like that to you, but it's merely a report describing a technical issue. Complete with a link to an example and with screenshots.

Yeah, I totally understand it was an issue but saying to me "can you not do it next time" isn't really a viable solution. CSS changes have to be made sometimes to fix issues - in this case an issue with the player scaling. I try to let users know in the changelog. It's not always worth bumping the major version for such minor things and even if I did it won't solve the issue for you; knowing what the CSS changes were. The only way currently is to look at the PR that I usually do a version bump and filtering for the .scss files.

I'd suspect your issue is to do with Flexbox now being used for the players container. You could simply add flex-direction: column; to your CSS for the .plyr container and that should resolve it. This is something I can add to the main style sheet in v3.5.8 without causing issues I think but it was something I would never have known would be an issue and for the majority of folk using the player either use the stylesheet as-is or would know the issue.

@tobireif
Copy link
Author

Yep, in the meantime I found that display: flex got added in that release.

@tobireif
Copy link
Author

tobireif commented Feb 10, 2020

In case someone else has the same issue: This is what I did to fix it for the player on the linked page:

In my .plyr class I added flex-direction: column; and align-items: start;. In my .plyr__controls class I added width: 100%;. In my video class I added border: none;.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants