feat: add HTML tooltip component - #40
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a comprehensive HTML tooltip component to the media player, standardizing the Portal API across both Tooltip and Popover components.
- Implements a new HTML tooltip component with floating UI positioning and accessibility features
- Updates React components to use consistent Portal API with
rootIdinstead ofidprops - Adds tooltip functionality to play/pause and fullscreen buttons in the HTML media skin
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/react/src/skins/default/styles.ts | Removes media-container class from styles |
| packages/react/react/src/skins/default/MediaSkinDefault.tsx | Updates Portal components to use rootId prop |
| packages/react/react/src/components/Tooltip.tsx | Changes Portal API from container/id to root/rootId |
| packages/react/react/src/components/Popover.tsx | Updates Portal API to match Tooltip component |
| packages/html/html/src/utils/element-utils.ts | Adds unique ID generator utility function |
| packages/html/html/src/skins/media-skin-default.ts | Integrates tooltip components into media player template |
| packages/html/html/src/components/media-tooltip.ts | Implements complete HTML tooltip component with floating UI |
| packages/html/html/src/components/media-popover.ts | Adds Portal component and updates positioning |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| computePosition(trigger, popup, { | ||
| placement, | ||
| middleware, | ||
| }).then(({ x, y, middlewareData }: { x: number; y: number; middlewareData: any }) => { |
There was a problem hiding this comment.
The middlewareData parameter should have a more specific type instead of any. Consider defining an interface or using the proper type from @floating-ui/dom.
|
|
||
| const triggerElement = this.firstElementChild as HTMLElement; | ||
| if (triggerElement) { | ||
| triggerElement.setAttribute('aria-describedby', 'tooltip'); |
There was a problem hiding this comment.
The aria-describedby value should reference a unique ID rather than the generic string 'tooltip'. Consider using uniqueId() to generate a proper reference.
This pull request introduces a new tooltip component to the HTML package and refactors how popover and tooltip portals are handled in both the HTML and React packages. The changes enable more flexible, portal-based positioning for overlays like tooltips and popovers, and add a robust tooltip experience to the default media skin.
New Tooltip Component and Portal Refactor:
media-tooltipweb component suite, including root, trigger, portal, positioner, popup, and arrow elements, with support for floating positioning and accessibility features.Portal Handling Improvements:
uniqueIdutility for generating unique element IDs, used to link portals to their containers.media-popovercomponents to use a dedicatedmedia-popover-portalelement for rendering overlays into a specific container, improving layering and positioning. [1] [2] [3] [4] [5] [6]React Component Consistency:
PopoverPortalandTooltipPortalprops to userootandrootIdinstead ofcontainerandid, matching the HTML component API for portal targeting. [1] [2] [3] [4]Other Notable Changes:
z-indexfrommedia-popover-positionerin favor of portal-based stacking.These changes collectively improve overlay component flexibility, accessibility, and maintainability across both HTML and React implementations.