-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add fullscreen control methods (requestFullscreen, exitFullscreen, getFullscreen) #34
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
Conversation
…en, getFullscreen) Added three new methods to VimeoPlayer for fullscreen control: - requestFullscreen(): Enter fullscreen mode - exitFullscreen(): Exit fullscreen mode - getFullscreen(): Get current fullscreen state Updated example app to demonstrate fullscreen functionality with auto-exit demo.
🦋 Changeset detectedLatest commit: 37a79e3 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds fullscreen control to the Vimeo player by introducing three public methods— Changes
Sequence DiagramsequenceDiagram
participant App
participant VimeoPlayer
participant Controller
participant WebView
participant PlayerAPI
App->>VimeoPlayer: requestFullscreen()
VimeoPlayer->>Controller: requestFullscreen()
alt Native/Web controller
Controller->>PlayerAPI: requestFullscreen()
PlayerAPI-->>Controller: void
else WebView controller
Controller->>WebView: executeCommand('requestFullscreen')
WebView->>PlayerAPI: request fullscreen
PlayerAPI-->>WebView: ✓
WebView-->>Controller: void
end
Controller-->>VimeoPlayer: Promise<void>
VimeoPlayer-->>App: Promise<void>
App->>VimeoPlayer: getFullscreen()
VimeoPlayer->>Controller: getFullscreen()
Controller->>PlayerAPI: getFullscreen()
PlayerAPI-->>Controller: boolean
Controller-->>VimeoPlayer: Promise<boolean>
VimeoPlayer-->>App: Promise<boolean>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying react-native-vimeo-bridge-example with
|
| Latest commit: |
37a79e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e1de1a01.react-native-vimeo-bridge-example.pages.dev |
| Branch Preview URL: | https://feat-fullscreen-control.react-native-vimeo-bridge-example.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
example/src/App.tsx (1)
236-242: Consider cleanup for setTimeout to follow best practices.The auto-exit timeout is not cleaned up if the component unmounts during the 3-second delay. While this is unlikely to cause issues since it only calls player methods (not state updates), adding cleanup would be more robust.
Apply this pattern:
<TouchableOpacity style={[styles.button, styles.fullscreenButton]} onPress={async () => { await player.requestFullscreen(); - setTimeout(async () => { + const timeoutId = setTimeout(async () => { await player.exitFullscreen(); }, 3000); + + // Optional: store timeoutId in a ref and clear on unmount }} >Or use a
useRefto store the timeout ID and clear it in auseEffectcleanup function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/cyan-lights-wait.md(1 hunks).gitignore(1 hunks)example/src/App.tsx(6 hunks)src/VimeoView.tsx(1 hunks)src/module/VimeoPlayer.ts(1 hunks)src/module/WebVimeoPlayerController.ts(1 hunks)src/module/WebviewVimeoPlayerController.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
src/VimeoView.tsx (1)
176-178: LGTM! Fullscreen commands properly integrated.The three new fullscreen commands follow the established pattern for player commands and correctly delegate to the underlying Vimeo Player API methods.
src/module/WebVimeoPlayerController.ts (1)
150-160: LGTM! Fullscreen methods follow established patterns.The implementation correctly:
- Uses optional chaining for safe delegation to the player instance
- Provides appropriate fallback value (
false) forgetFullscreen()- Maintains consistency with existing getter/setter methods like
getMuted()andsetMuted().changeset/cyan-lights-wait.md (1)
1-34: LGTM! Clear and accurate documentation.The changeset appropriately documents the new fullscreen control methods with a practical example demonstrating the usage pattern.
src/module/VimeoPlayer.ts (1)
133-143: LGTM! Fullscreen API properly exposed.The three new methods correctly delegate to the controller with:
- Appropriate optional chaining for null-safe access
- Proper return types and fallback values
- Consistency with the existing API pattern
example/src/App.tsx (2)
38-38: LGTM! Fullscreen event properly subscribed.The fullscreen event watcher follows the same pattern as other event subscriptions in the component.
128-128: LGTM! UI layout and styles properly configured.The ScrollView configuration and new fullscreen section styles are consistent with the existing component patterns and properly accommodate the new fullscreen controls.
Also applies to: 272-276, 415-433
src/module/WebviewVimeoPlayerController.ts (1)
89-95: LGTM! Fullscreen action methods correctly implemented.Both
requestFullscreen()andexitFullscreen()properly delegate toexecuteCommandwithout waiting for a result, consistent with other action methods likeplay()andpause().
Added three new methods to VimeoPlayer for fullscreen control:
Updated example app to demonstrate fullscreen functionality with auto-exit demo.
Related discussion:
Summary by CodeRabbit