Skip to content

fix: use popover core in react popover#208

Merged
luwes merged 2 commits intomainfrom
react-popover
Nov 25, 2025
Merged

fix: use popover core in react popover#208
luwes merged 2 commits intomainfrom
react-popover

Conversation

@luwes
Copy link
Copy Markdown
Collaborator

@luwes luwes commented Nov 24, 2025

This PR makes the React Popover component use the Core component removing duplicated code.

@luwes luwes requested a review from Copilot November 24, 2025 19:52
@luwes luwes self-assigned this Nov 24, 2025
@vercel
Copy link
Copy Markdown

vercel bot commented Nov 24, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
vjs-10-demo-html Ready Ready Preview Comment Nov 24, 2025 8:11pm
vjs-10-demo-next Ready Ready Preview Comment Nov 24, 2025 8:11pm
vjs-10-demo-react Ready Ready Preview Comment Nov 24, 2025 8:11pm
vjs-10-website Ready Ready Preview Comment Nov 24, 2025 8:11pm

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the React Popover component to use the Core Popover component instead of maintaining duplicate implementation logic. The changes consolidate hover behavior, transition handling, and positioning logic into the core package while the React component now acts as a thin wrapper.

Key changes:

  • React Popover now imports and delegates to @videojs/core Popover component
  • Renamed open to _open in core state to indicate internal usage
  • Wrapped hidePopover() call in requestAnimationFrame for proper animation timing

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/react/src/components/Popover.tsx Refactored to use component factory pattern with Core Popover, removing ~150 lines of duplicate hover/positioning logic
packages/html/src/elements/popover.ts Exported PopoverState type for external usage
packages/core/src/components/popover.ts Renamed open to _open and added requestAnimationFrame wrapper for hidePopover transitions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/core/src/components/popover.ts
@vercel vercel bot temporarily deployed to Preview – vjs-10-demo-html November 24, 2025 20:10 Inactive
@vercel vercel bot temporarily deployed to Preview – vjs-10-demo-react November 24, 2025 20:10 Inactive
@vercel vercel bot temporarily deployed to Preview – vjs-10-demo-next November 24, 2025 20:10 Inactive
@vercel vercel bot temporarily deployed to Preview – vjs-10-website November 24, 2025 20:11 Inactive
@luwes luwes merged commit 99fef78 into main Nov 25, 2025
7 checks passed
@luwes luwes deleted the react-popover branch November 25, 2025 23:27
@github-actions github-actions bot mentioned this pull request Nov 24, 2025
@github-actions github-actions bot mentioned this pull request Feb 26, 2026
@github-actions github-actions bot mentioned this pull request Mar 10, 2026
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.

2 participants