Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Document Picture-in-Picture: Use width/height instead of initialAspectRatio #8270

Merged
merged 1 commit into from
May 31, 2023

Conversation

beaufortfrancois
Copy link
Contributor

The initialAspectRatio option was removed from the Document Picture-in-Picture spec in favour of width and height, so we should use them. See WICG/document-picture-in-picture#36

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #8270 (ebf472f) into main (1491d71) will increase coverage by 82.36%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           main    #8270       +/-   ##
=========================================
+ Coverage      0   82.36%   +82.36%     
=========================================
  Files         0      112      +112     
  Lines         0     7474     +7474     
  Branches      0     1800     +1800     
=========================================
+ Hits          0     6156     +6156     
- Misses        0     1318     +1318     
Impacted Files Coverage Δ
src/js/player.js 90.27% <ø> (ø)

... and 111 files with indirect coverage changes

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

@mister-ben
Copy link
Contributor

Thanks. Seems setting width and height has the same issue as with aspect ratio of sizing the outer pip window rather than setting the content size.

@mister-ben mister-ben added the needs: LGTM Needs one or more additional approvals label May 12, 2023
@beaufortfrancois
Copy link
Contributor Author

Shall we merge?

@misteroneill misteroneill merged commit 9e1e29d into videojs:main May 31, 2023
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
needs: LGTM Needs one or more additional approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants