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: player resize event #4800

Merged
merged 4 commits into from
Dec 14, 2017
Merged

Conversation

sivapalan
Copy link
Contributor

@sivapalan sivapalan commented Dec 10, 2017

Description

Fixes #4629, and also handles multi-dimensional resizing correctly. I.e. playerresize event is only triggered once when width and height is changed at once (using Component#dimensions).

Specific Changes proposed

Trigger a playerresize event in Player#dimension if Player.isReady_ and skipListeners is falsy.

skipListeners is added as an optional parameter to the following methods: Player#width, Player#height, and Player#dimension. This parameter can be used to skip the playerresize event trigger.

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 (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

Trigger a playerresize event when the player is resized using the API

Fixes videojs#4629
Allows us to avoid triggering the resize event twice when setting both width and height at once
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

I totally forgot that we had skipListeners feature. Good catch!

@gkatsev gkatsev changed the title fix: player resize event feat: player resize event Dec 11, 2017
@gkatsev
Copy link
Member

gkatsev commented Dec 11, 2017

Given that we're adding a new event, this would be a feat.

@gkatsev gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. needs: LGTM Needs one or more additional approvals labels Dec 11, 2017
@gkatsev
Copy link
Member

gkatsev commented Dec 11, 2017

Also, thanks for adding tests!

Copy link
Contributor

@ldayananda ldayananda left a comment

Choose a reason for hiding this comment

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

Nicely done!

@ldayananda ldayananda added confirmed and removed needs: LGTM Needs one or more additional approvals labels Dec 11, 2017
@gkatsev gkatsev merged commit e0ed0b5 into videojs:master Dec 14, 2017
@sivapalan sivapalan deleted the player_resize_event branch December 15, 2017 12:50
@gkatsev gkatsev mentioned this pull request Jan 8, 2018
10 tasks
gkatsev added a commit that referenced this pull request Jan 9, 2018
gkatsev added a commit that referenced this pull request Jan 30, 2018
Use ResizeObserver when available for better and more performant resizing information, otherwise, fall back to a throttled resize event on an iframe that's the size of the player.
Allows a video.js user to disable this by setting resizeManager: false as an option since the component will not be initialized.

Add a debounce util.

This reverts #4800 (e0ed0b5) because we end up getting two playerresize events with the dimension methods now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an event to signal that the player was resized using the API e.g. player.dimensions()
3 participants