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

feat: Use picture el for poster #7865

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

mister-ben
Copy link
Contributor

Description

Use a picture element instead of background image for the poster. Resolves issues some have had with the PosterImage and video el loading the poster differently.

Leaves scope to add support for source set for multiple image formats later, which could be done as a backwards-compatible minor.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #7865 (a32de1f) into next (9e95740) will increase coverage by 0.05%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##             next    #7865      +/-   ##
==========================================
+ Coverage   81.91%   81.97%   +0.05%     
==========================================
  Files         111      111              
  Lines        7344     7356      +12     
  Branches     1771     1776       +5     
==========================================
+ Hits         6016     6030      +14     
+ Misses       1328     1326       -2     
Impacted Files Coverage Δ
src/js/player.js 89.76% <80.00%> (+0.21%) ⬆️
src/js/poster-image.js 75.60% <86.66%> (+1.41%) ⬆️
src/js/tech/tech.js 83.71% <0.00%> (+0.28%) ⬆️

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

@@ -46,12 +46,16 @@ class PosterImage extends ClickableComponent {
* The element that gets created.
*/
createEl() {
const el = Dom.createEl('div', {
const crossOrigin = this.player_.options_.crossOrigin || this.player_.options_.crossorigin || null;
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the crossOrigin getter?

Suggested change
const crossOrigin = this.player_.options_.crossOrigin || this.player_.options_.crossorigin || null;
const crossOrigin = this.player_.crossOrigin() ?? null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The poster can be set up before the getter will return a correct value, as it's dependent on the tech

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's right. However, we should probably still use the getter first because otherwise, the value may be out of sync with the rest of the system.

src/js/player.js Outdated
Comment on lines 842 to 844
if (this.posterImage) {
this.posterImage.$('img').crossOrigin = value;
}
Copy link
Member

Choose a reason for hiding this comment

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

thought: maybe PosterImage should have a crossOrigin getter/setter so that this piece can be moved into it?

if (this.posterImage) {
  this.posterImage.crossOrigin(value);
}

If we do, we'll probably want to update the createEl below to use this option.

@mister-ben
Copy link
Contributor Author

Updated to add a getter/setter to posterImage. The getter gets posterImage's value if the el has been created, otherwise the player/tech's vlaue if the tech is ready, otherwise from the player options.
Updated the player setter to accept null as a value, which is needed to unset the value.

@misteroneill misteroneill merged commit b5fefde into videojs:next Aug 22, 2022
misteroneill pushed a commit that referenced this pull request Nov 23, 2022
BREAKING CHANGE: This changes the DOM structure used for the video poster.
misteroneill pushed a commit that referenced this pull request Nov 23, 2022
BREAKING CHANGE: This changes the DOM structure used for the video poster.
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
BREAKING CHANGE: This changes the DOM structure used for the video poster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants