feat(video): align React component with Marko and shaka-player v5#570
feat(video): align React component with Marko and shaka-player v5#570HenriqueLimas merged 2 commits intomainfrom
Conversation
Refactor React video component to use declarative approach with createPortal for Shaka controls integration. Implement missing control components (MuteButton, FullscreenButton, CurrentTime, TotalTime, RemainingTime, Report, Captions) using reusable useShakaControl hook with flushSync for synchronous rendering. Fix icon re-rendering issue and autoplay behavior to match Marko implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: aacc479 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
There was a problem hiding this comment.
Pull request overview
Aligns the React EbayVideo implementation with the Marko version and Shaka Player v5 by refactoring Shaka UI integration to a more declarative, portal-based approach and adding missing control elements.
Changes:
- Refactors
@ebay/ui-core-reactvideo to usecreatePortal+ a reusableuseShakaControlhook for Shaka UI controls. - Adds an accessible play-button wrapper in Marko and corresponding Skin button-reset styles.
- Updates icon provider behavior to prevent re-render issues and refreshes/extends tests and snapshots.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/skin/src/sass/video/video.scss | Adds reset styles for the Shaka play button within the video player. |
| packages/skin/dist/video/video.css | Regenerated dist CSS to include the new play button reset styles. |
| packages/ebayui-core/src/components/ebay-video/test/snapshots/test.server.js.snap | Updates snapshots for play icon markup (adds width="64"). |
| packages/ebayui-core/src/components/ebay-video/index.marko | Updates hidden icon markup to include explicit width=64. |
| packages/ebayui-core/src/components/ebay-video/component.ts | Wraps the play icon in an accessible <button> for keyboard/a11y support. |
| packages/ebayui-core-react/src/ebay-video/video.tsx | Major refactor: Shaka overlay setup, portal play button, new controls, autoplay event semantics, layout support. |
| packages/ebayui-core-react/src/ebay-video/reportButton.tsx | Removes legacy ReportButton implementation (replaced by Shaka control + portal). |
| packages/ebayui-core-react/src/ebay-video/controls/use-shaka-control.ts | Introduces reusable hook to register Shaka UI elements and bridge events/state into React portals. |
| packages/ebayui-core-react/src/ebay-video/controls/total-time-control.tsx | Adds Total Time control rendered via portal into Shaka UI. |
| packages/ebayui-core-react/src/ebay-video/controls/time-utils.ts | Adds shared time formatting helper used by time-based controls. |
| packages/ebayui-core-react/src/ebay-video/controls/report-button-control.tsx | Adds Report control rendered via portal into Shaka UI. |
| packages/ebayui-core-react/src/ebay-video/controls/remaining-time-control.tsx | Adds Remaining Time control for compact layout and live/VOD handling. |
| packages/ebayui-core-react/src/ebay-video/controls/mute-button-control.tsx | Adds Mute control with icon swapping based on muted/volume state. |
| packages/ebayui-core-react/src/ebay-video/controls/fullscreen-button-control.tsx | Adds Fullscreen control with icon swapping and support detection. |
| packages/ebayui-core-react/src/ebay-video/controls/current-time-control.tsx | Adds Current Time / LIVE control including “skip to live” behavior. |
| packages/ebayui-core-react/src/ebay-video/controls/captions-control.tsx | Registers Shaka’s built-in text selection control for captions. |
| packages/ebayui-core-react/src/ebay-video/controls.tsx | Removes legacy imperative Shaka custom controls implementation. |
| packages/ebayui-core-react/src/ebay-video/const.ts | Adds DEFAULT_SPINNER_TIMEOUT; minor config cleanup. |
| packages/ebayui-core-react/src/ebay-video/tests/render.spec.tsx | Updates rendering assertions to reflect new sizing/styling behavior. |
| packages/ebayui-core-react/src/ebay-video/tests/index.stories.tsx | Updates story defaults (removes a11yLoadText, width change). |
| packages/ebayui-core-react/src/ebay-video/tests/index.spec.tsx | Expands tests for portal play button, icon width, and autoplay behavior. |
| packages/ebayui-core-react/src/ebay-icon/icon.tsx | Adjusts Skin class naming to correctly reflect “-colored” sizing classes. |
| packages/ebayui-core-react/src/ebay-icon/context.tsx | Makes IconContext Set stable across re-renders via useRef. |
| .changeset/video-portal-play-button.md | Adds a changeset for patch releases across affected packages. |
Comments suppressed due to low confidence (1)
packages/ebayui-core-react/src/ebay-video/video.tsx:245
isAutoPauseis never set totrue. In theaction === "pause"branch you pause the video but don’t mark it as an automatic pause, soonPause(..., { auto })will always reportauto: falsefor action-driven pauses. Consider settingsetIsAutoPause(true)before callingvideoRef.current.pause()(and resetting after emitting).
useEffect(() => {
if (!videoRef.current) return;
switch (action) {
case "play":
setIsAutoPlay(true);
videoRef.current.play();
break;
case "pause":
videoRef.current.pause();
break;
| playButton.classList.add("shaka-play-button"); | ||
| playButton.setAttribute("aria-label", this.input.a11yPlayText || "Click to play"); |
There was a problem hiding this comment.
Click is more focused on mouse users. Lets rename it to "play". Need refactor across multiple places on this wording.
| playButton.classList.add("shaka-play-button"); | ||
| playButton.setAttribute("aria-label", this.input.a11yPlayText || "Click to play"); | ||
| playButton.onclick = () => { | ||
| this.video.play(); |
| } | ||
| }; |
| style={style} | ||
| poster={thumbnail} | ||
| playsInline | ||
| muted={muted || false} |
| {shakaControlsContainer && | ||
| createPortal( | ||
| <div className="shaka-play-button-container"> | ||
| <button className="shaka-play-button" aria-label={a11yPlayText} onClick={handleOnPlayClick}> |
| const handleOnPlayClick = () => { | ||
| // Remove play button from React tree before shaka player removes it | ||
| setShowInitialPlayButton(false); | ||
| videoRef.current.play(); | ||
| }; |
| --- | ||
| "@ebay/ui-core-react": patch | ||
| "@ebay/ebayui-core": patch | ||
| "@ebay/skin": patch | ||
| --- |
saiponnada
left a comment
There was a problem hiding this comment.
Few things I noticed. The focus ring is very light and I belive this is something we might not have control on(coming from shaka player). On marko story, the play button is kb focusable but doesnt invoke the action on pressing enter(React version works correctly).
| playButton.classList.add("shaka-play-button"); | ||
| playButton.setAttribute("aria-label", this.input.a11yPlayText || "Click to play"); |
There was a problem hiding this comment.
Click is more focused on mouse users. Lets rename it to "play". Need refactor across multiple places on this wording.
…aults - Fix alignSeekbar null safety in React (remove non-null assertions, guard rangeContainer) - Fix muted prop default: `muted !== false` to match Marko behavior - Set isAutoPause before programmatic pause via action prop in React - Add explicit type="button" to play button in both Marko and React - Update a11yPlayText default from "Click to play" to "Play" (WCAG-aligned) - Remove dead defaultVideoConfig export from React const.ts Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Description
createPortalfor Shaka controls integration.MuteButton,FullscreenButton,CurrentTime,TotalTime,RemainingTime,Report,Captions) using reusableuseShakaControlhook.Notes
Screenshots
Checklist