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.

Sign up for GitHub

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

Jump to bottom

feat: Add support for Document Picture-in-Picture #4969

Merged
merged 14 commits into from
Feb 10, 2023
Merged

feat: Add support for Document Picture-in-Picture #4969

merged 14 commits into from
Feb 10, 2023

Conversation

beaufortfrancois
Copy link
Contributor

@beaufortfrancois beaufortfrancois commented Feb 6, 2023 •
edited by avelad
Loading

This PR adds experimental support for the Document Picture-in-Picture API in the Shaka Player.

I'm making some assumptions about how Shaka Player works and I'd love your review on this. Thank you!

Closes #4964

image
@beaufortfrancois beaufortfrancois mentioned this pull request Feb 6, 2023
Closed
@beaufortfrancois beaufortfrancois changed the title Add support for Document Picture-in-Picture Feb 6, 2023
@avelad avelad added type: enhancement New feature or request component: UI The issue involves the Shaka Player UI priority: P3 Useful but not urgent labels Feb 6, 2023
@avelad avelad added this to the v4.4 milestone Feb 6, 2023
@avelad
Copy link
Collaborator

avelad commented Feb 6, 2023

@beaufortfrancois Please, fix the build errors. Thanks!
avelad
avelad requested changes Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@avelad avelad left a comment

Choose a reason for hiding this comment

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

I've been testing the functionality, several things I've seen:

  • The dimensions that the player occupies in the window becomes 0, this can cause problems in some websites.
  • The label of whether you are in PiP mode or not PiP is not updating correctly.
  • Missing to detect if the functionality is allowed (see isPipAllowed_)
@beaufortfrancois
Copy link
Contributor Author

beaufortfrancois commented Feb 6, 2023 •
edited
Loading

  • The dimensions that the player occupies in the window becomes 0, this can cause problems in some websites.

I agree and that's something I'd like to discuss. Adding a container may be enough. I'm not sure though.

  • The label of whether you are in PiP mode or not PiP is not updating correctly.

Fixed. Thanks!

  • Missing to detect if the functionality is allowed (see isPipAllowed_)

I forgot to upload that change earlier. Thank you!
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023 •
edited
Loading

Incremental code coverage: 23.68%
@avelad
Copy link
Collaborator

avelad commented Feb 6, 2023

  • The dimensions that the player occupies in the window becomes 0, this can cause problems in some websites.

I agree and that's something I'd like to discuss. Adding a container may be enough. I'm not sure though.

@joeyparrish , what is your opinion about that?
avelad
avelad previously approved these changes Feb 7, 2023
View reviewed changes
Copy link
Collaborator

@avelad avelad left a comment

Choose a reason for hiding this comment

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

LGTM!
theodab
theodab requested changes Feb 9, 2023
View reviewed changes
ui/pip_button.js Show resolved Hide resolved
ui/pip_button.js Show resolved Hide resolved
@avelad avelad requested a review from theodab February 9, 2023 09:02
theodab
theodab requested changes Feb 9, 2023
View reviewed changes
docs/tutorials/ui-customization.md Outdated Show resolved Hide resolved
@avelad avelad requested a review from theodab February 9, 2023 09:24
theodab
theodab previously approved these changes Feb 9, 2023
View reviewed changes
avelad
avelad previously approved these changes Feb 9, 2023
View reviewed changes
joeyparrish
joeyparrish requested changes Feb 9, 2023
View reviewed changes
demo/close_button.js Outdated Show resolved Hide resolved
demo/close_button.js Outdated Show resolved Hide resolved
ui/pip_button.js Outdated Show resolved Hide resolved
@avelad avelad dismissed stale reviews from theodab and themself via 3d06618 February 9, 2023 17:40
@joeyparrish
Copy link
Member

Looks like the externs for documentPictureInPicture need to be updated to make that an implementation of EventTarget. I'll make the revision.
@joeyparrish
Copy link
Member

Some other type information is missing, too, so I'm adding all of it.
@beaufortfrancois
Copy link
Contributor Author

@joeyparrish Thanks for fixing externs!
@joeyparrish joeyparrish merged commit 3828fd6 into shaka-project:main Feb 10, 2023
@github-actions github-actions bot mentioned this pull request Feb 10, 2023
Merged
@beaufortfrancois
Copy link
Contributor Author

Do you folks know when this feature will be available in next shaka player release?
@beaufortfrancois
Copy link
Contributor Author

@joeyparrish @avelad
Do you know which release will have Document Picture-in-Picture support? Any rough ETA?
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: UI The issue involves the Shaka Player UI priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
4 participants
@beaufortfrancois @avelad @joeyparrish @theodab