Remove non-functional VideoPlayer showControlsWhenPaused prop - #1388
Conversation
🦋 Changeset detectedLatest commit: 137bc23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
🟢 No design token changes found |
🟢 No unit test coverage changes foundAll components and hooks with tests maintain the same coverage as the main branch. |
There was a problem hiding this comment.
Pull request overview
This PR updates the VideoPlayer component to properly support (and type) the showControlsWhenPaused prop, allowing consumers to hide the control bar while paused, and adds Storybook + test coverage around the new behavior.
Changes:
- Added
showControlsWhenPaused?: booleantoVideoPlayerprops with a default oftrue. - Updated controls bar visibility logic to hide the controls bar when paused if
showControlsWhenPaused={false}. - Added Storybook control and new unit tests covering paused vs playing behavior.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/VideoPlayer/VideoPlayer.tsx | Adds the showControlsWhenPaused prop and updates controls-bar rendering logic. |
| packages/react/src/VideoPlayer/VideoPlayer.test.tsx | Adds tests asserting controls are hidden while paused (but shown while playing) when the prop is false. |
| packages/react/src/VideoPlayer/VideoPlayer.stories.tsx | Adds a Storybook boolean control + default arg for showControlsWhenPaused. |
| .changeset/smart-wolves-pause.md | Adds a changeset entry describing the update. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
🟢 Bundle size report
|
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@rezrah I think we could simply remove the prop completely instead of marking it as deprecated, as it hasn't been working since |
Agree, we should just remove it unless we have a 100% working solution for it (no CLS impact). I'm a bit unclear on whether we need to remove or restore original functionality though, can you confirm? If it's genuinely not needed, let's remove it. |
|
It is safe to remove since it has been non-functional. There is a temporary solution to work around VideoPlayer limitations for that use case, and https://github.com/github/brand-experience/issues/383 is planned. 👍 |
Summary
showControlsWhenPausedused to control whetherVideoPlayerrendered its controls while paused. That behavior was removed in0.49.0because hiding/showing the controls changed the player height on play/pause, causing layout shift.Since the prop has already been non-functional, this PR removes the remaining public API and documentation references instead of reintroducing behavior or keeping a deprecated no-op.
List of notable changes:
showControlsWhenPausedfrom theVideoPlayerTypeScript prop surfaceshowControlsWhenPausedfrom Storybook controlsshowControlsWhenPausedfrom the VideoPlayer docs props tableSteps to test:
npm run build:lib.npm run test --workspace=packages/react -- --testPathPatterns="VideoPlayer".npm run check --workspace=packages/react.npm run build:next-docs.showControlsWhenPausedis no longer listed in the VideoPlayer docs, Storybook controls, or generated TypeScript declarations.Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshotslabel to the PR)Reviewer checklist:
Screenshots: