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

[video] add fullscreen enter and exit events #30922

Merged
merged 16 commits into from
Aug 20, 2024

Conversation

fobos531
Copy link
Contributor

@fobos531 fobos531 commented Aug 9, 2024

Why

Adds fullscreen enter/exit events across all platforms. Motivated by bluesky-social/social-app#4907, therefore credits to @haileyok

How

Just followed the original implementation from Hailey, with small difference being the naming of the actual event which follows the PiP event naming, as well support added on web.

Test Plan

CleanShot.2024-08-09.at.14.05.24.mp4
CleanShot.2024-08-09.at.14.06.24.mp4

Untested on web & apple TV.

Checklist

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 9, 2024
Copy link
Member

@behenate behenate left a comment

Choose a reason for hiding this comment

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

Looks good 😄 Thank you!

Comment on lines 161 to 168
/**
* Handler for an event emitted when the player enters the fullscreen mode.
*/
onFullscreenEnter(): void;
/**
* Handler for an event emitted when the player exits the fullscreen mode.
*/
onFullscreenExit(): void;
Copy link
Member

Choose a reason for hiding this comment

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

They are VideoView events, not VideoPlayer events 😉 You can remove this (and rebuild files).

Suggested change
/**
* Handler for an event emitted when the player enters the fullscreen mode.
*/
onFullscreenEnter(): void;
/**
* Handler for an event emitted when the player exits the fullscreen mode.
*/
onFullscreenExit(): void;

Copy link
Contributor Author

@fobos531 fobos531 Aug 9, 2024

Choose a reason for hiding this comment

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

Hey @tsapeta , that makes sense. I believe just removing this is not enough and that it needs to be added to the VideoView.web.tsx, like so: 59fdd57 (#30922)

I followed @behenate 's PIP implementation as seen here: https://github.com/expo/expo/pull/30524/files#diff-72705cdf87b0dfe8ecd5783a1ffd65f9e6257b81e046c967f4535b75b89c9835R74

Let me know if this looks good and whether anything else needs to changed

@fobos531 fobos531 requested a review from tsapeta August 9, 2024 13:39
@haileyok
Copy link
Contributor

haileyok commented Aug 9, 2024

@fobos531 Cool!! Thanks for doing this! 🎉

@fobos531 fobos531 requested a review from aleqsio August 14, 2024 10:21
packages/expo-video/src/VideoView.web.tsx Outdated Show resolved Hide resolved
@fobos531 fobos531 requested a review from tsapeta August 19, 2024 09:23
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
@tsapeta tsapeta merged commit df90d60 into expo:main Aug 20, 2024
10 checks passed
@fobos531 fobos531 deleted the @fobos531/fullscreen-video-events branch August 21, 2024 08:35
behenate pushed a commit that referenced this pull request Sep 6, 2024
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants