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

fix: Don't use copyStyleSheets with documentPIP #8314

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

beaufortfrancois
Copy link
Contributor

@beaufortfrancois beaufortfrancois commented Jun 8, 2023

Document Picture-in-Picture API owners are removing copyStyleSheets option as it can be done in JavaScript. This PR removes this option and copy all style sheets internally to the PiP window.

@mister-ben Please review.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #8314 (77becf3) into main (6c4997d) will increase coverage by 0.04%.
The diff coverage is 57.14%.

❗ Current head 77becf3 differs from pull request most recent head 873d0c7. Consider uploading reports for the commit 873d0c7 to get more accurate results

@@            Coverage Diff             @@
##             main    #8314      +/-   ##
==========================================
+ Coverage   82.57%   82.62%   +0.04%     
==========================================
  Files         112      113       +1     
  Lines        7497     7578      +81     
  Branches     1809     1820      +11     
==========================================
+ Hits         6191     6261      +70     
- Misses       1306     1317      +11     
Impacted Files Coverage Δ
src/js/player.js 90.89% <ø> (+0.07%) ⬆️
src/js/utils/dom.js 64.28% <57.14%> (-0.92%) ⬇️

... and 25 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I'm not really sure I agree that it makes any more sense for each player library to manage copying stylesheets, but if that's how it is...

The copy method would ideally not be exposed on the player, as the inline comment. We'll need to address test coverage too.

src/js/player.js Outdated Show resolved Hide resolved
@beaufortfrancois
Copy link
Contributor Author

The copy method would ideally not be exposed on the player, as the inline comment. We'll need to address test coverage too.

I've moved it and added some basic tests. Let me know if that looks good to you.

@beaufortfrancois
Copy link
Contributor Author

(gentle ping)

@mister-ben
Copy link
Contributor

@beaufortfrancois Tahnks for the updates. I've been out for a couple of weeks, back now and will check this week.

src/js/utils/dom.js Outdated Show resolved Hide resolved
src/js/utils/dom.js Outdated Show resolved Hide resolved
@mister-ben
Copy link
Contributor

We'll need to do something with this as Chrome Canary already remove copyStylesheets. @beaufortfrancois any comments re the suggested changes to make sure the order of stylesheets is preserved?

@beaufortfrancois
Copy link
Contributor Author

Thank you @mister-ben for all your feedback!

Copy link
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mister-ben mister-ben changed the title Don't use copyStyleSheets fix: Don't use copyStyleSheets wirth documentPIP Jul 7, 2023
@mister-ben mister-ben changed the title fix: Don't use copyStyleSheets wirth documentPIP fix: Don't use copyStyleSheets with documentPIP Jul 7, 2023
@mister-ben mister-ben added the needs: LGTM Needs one or more additional approvals label Jul 7, 2023
@mister-ben mister-ben removed the needs: LGTM Needs one or more additional approvals label Jul 7, 2023
@mister-ben mister-ben merged commit 8dd98f6 into videojs:main Jul 7, 2023
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

Successfully merging this pull request may close these issues.

3 participants