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

Add Picture-in-Picture API methods #6001

Merged
merged 5 commits into from
Jun 11, 2019

Conversation

beaufortfrancois
Copy link
Contributor

Description

Following #5824 (comment), this PR adds support for some Picture-in-Picture methods described in the spec and article

Specific Changes proposed

This PR is about entering and leaving Picture-in-Picture in the player. It also makes sure that we can listen to the enterpictureinpicture and leavepictureinpicture events on the player.

Requirements Checklist

  • Feature implemented / Bug fixed [Support for picture-in-picture API #5824]
  • 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

@welcome
Copy link

welcome bot commented May 21, 2019

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@gkatsev
Copy link
Member

gkatsev commented May 21, 2019

Can an "abstract"/empty method be added to tech.js, for example https://github.com/videojs/video.js/blob/master/src/js/tech/tech.js#L767-L772? It's needed because if another tech that doesn't implement this method yet ends up being used (for example videojs-flash), we don't want it to throw an error.

@beaufortfrancois
Copy link
Contributor Author

beaufortfrancois commented May 22, 2019

Can an "abstract"/empty method be added to tech.js, for example https://github.com/videojs/video.js/blob/master/src/js/tech/tech.js#L767-L772? It's needed because if another tech that doesn't implement this method yet ends up being used (for example videojs-flash), we don't want it to throw an error.

@gkatsev Done in 44d09fc

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.

Works great, thanks!

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label May 31, 2019
@gkatsev
Copy link
Member

gkatsev commented May 31, 2019

We try to have two core contributors review PRs before they're merged but this is definitely on the way! Thanks again!

@beaufortfrancois
Copy link
Contributor Author

@gkatsev Hopefully it's gonna be OK then. Thanks for reviewing ;)

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.

Just realized promises may not be available by default.

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

@gkatsev I think we're good now. WDYT?

src/js/tech/tech.js Outdated Show resolved Hide resolved
@gkatsev gkatsev merged commit 83541dc into videojs:master Jun 11, 2019
@welcome
Copy link

welcome bot commented Jun 11, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@beaufortfrancois beaufortfrancois deleted the pip-api-methods branch June 12, 2019 06:56
*
* @listens focus
*/
handleFocus(event) {
Copy link
Member

Choose a reason for hiding this comment

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

@gkatsev Why we need handleFocus here? I thought it should be removed... and this.boundHandleKeyPress_ doesn't exist inside the player class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI. This change was not part of my proposed changes.

Copy link
Member

Choose a reason for hiding this comment

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

Hm... must've snuck in when I was fixing merge conflicts. I'll make sure that it's fixed in master. Nice find @gjanblaszczyk

Copy link
Member

Choose a reason for hiding this comment

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

PR to re-remove them: #6053

gkatsev added a commit that referenced this pull request Jun 17, 2019
gkatsev added a commit that referenced this pull request Jun 17, 2019
@GeoTimber
Copy link

Jsut a quick question, how do I disable the picture in picture button from the player init options?

@gkatsev
Copy link
Member

gkatsev commented Jul 22, 2019

@GeoTimber see the option here #6002 (comment)

@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019
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.

6 participants