Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
sampotts
requested review from
Copilot and
mihar-22
and removed request for
Copilot
Contributor
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (5)
Players (3)
Skins (16)
UI Components (21)
Sizes are marginal over the root entry point. ⚛️ @videojs/react(no changes) Presets (7)
Media (4)
Skins (14)
UI Components (18)
Sizes are marginal over the root entry point. 🧩 @videojs/core(no changes) Entries (6)
🏷️ @videojs/element(no changes) Entries (2)
📦 @videojs/store(no changes) Entries (3)
🔧 @videojs/utils(no changes) Entries (10)
📦 @videojs/spf(no changes) Entries (3)
ℹ️ How to interpretAll sizes are standalone totals (minified + brotli).
Run |
Contributor
CI Failure Diagnosis
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an explicit “volume availability” signal throughout the UI so skins can hide/disable volume controls (notably the volume popover on iOS/Mobile Safari where programmatic volume changes are unsupported), backed by a runtime capability probe and new tests/docs.
Changes:
- Introduces async runtime probing for
volumeAvailabilityin the corevolumefeature and updates tests accordingly. - Threads
availabilityintoMuteButtonandVolumeSliderstate +data-availabilityattrs across core/react/html implementations. - Gates React skins’ volume popover rendering and HTML popover trigger binding/visibility based on availability; updates docs with styling guidance.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/content/docs/reference/volume-slider.mdx | Documents data-availability and CSS patterns to hide unsupported volume sliders. |
| site/src/content/docs/reference/feature-volume.mdx | Updates examples to check volumeAvailability before rendering volume UI; adds HTML getter example. |
| packages/react/src/ui/volume-slider/volume-slider-root.tsx | Switches to VolumeSliderDataAttrs so data-availability is applied on the root. |
| packages/react/src/ui/volume-slider/tests/volume-slider.test.tsx | Adds coverage for data-availability propagation and render-state exposure. |
| packages/react/src/presets/video/skin.tsx | Gates volume popover rendering based on volumeAvailability; factors mute render into a shared function. |
| packages/react/src/presets/video/skin.tailwind.tsx | Same gating/refactor for the Tailwind video skin. |
| packages/react/src/presets/video/minimal-skin.tsx | Same gating/refactor for the minimal video skin. |
| packages/react/src/presets/video/minimal-skin.tailwind.tsx | Same gating/refactor for the Tailwind minimal video skin. |
| packages/react/src/presets/audio/skin.tsx | Same gating/refactor for the audio skin. |
| packages/react/src/presets/audio/skin.tailwind.tsx | Same gating/refactor for the Tailwind audio skin. |
| packages/react/src/presets/audio/minimal-skin.tsx | Same gating/refactor for the minimal audio skin. |
| packages/react/src/presets/audio/minimal-skin.tailwind.tsx | Same gating/refactor for the Tailwind minimal audio skin. |
| packages/html/src/ui/volume-slider/volume-slider-element.ts | Switches to VolumeSliderDataAttrs so data-availability is applied in HTML implementation. |
| packages/html/src/ui/popover/popover-element.ts | Adds trigger availability gating using trigger’s data-availability to hide/unbind popovers when unsupported. |
| packages/html/src/ui/popover/tests/popover-element.test.ts | Adds tests covering binding/visibility behavior for available vs unsupported triggers. |
| packages/core/src/dom/store/features/volume.ts | Replaces sync canSetVolume with async DOM probe to resolve volumeAvailability. |
| packages/core/src/dom/store/features/tests/volume.test.ts | Adds coverage for async probe resolution and unsupported detection behavior. |
| packages/core/src/core/ui/volume-slider/volume-slider-data-attrs.ts | Adds availability -> data-availability mapping for volume slider state attrs. |
| packages/core/src/core/ui/volume-slider/volume-slider-core.ts | Adds availability to slider state derived from media volumeAvailability. |
| packages/core/src/core/ui/volume-slider/tests/volume-slider-core.test.ts | Verifies availability is projected into VolumeSliderCore state. |
| packages/core/src/core/ui/mute-button/mute-button-core.ts | Adds availability to mute button state. |
| packages/core/src/core/ui/mute-button/mute-button-data-attrs.ts | Adds availability -> data-availability mapping for mute button state attrs. |
| packages/core/src/core/ui/mute-button/tests/mute-button-core.test.ts | Updates tests for new availability state on mute button. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+164
to
167
| return; | ||
| } | ||
|
|
||
| if (state.open) { |
Comment on lines
+142
to
+144
| const availableTriggerEl = this.#isTriggerAvailable(triggerEl) ? triggerEl : null; | ||
| this.toggleAttribute('hidden', !availableTriggerEl); | ||
| this.#syncTrigger(availableTriggerEl); |
Comment on lines
+48
to
+51
| detectVolumeAvailability().then((volumeAvailability) => { | ||
| if (signal.aborted) return; | ||
| set({ volumeAvailability }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a first-class “volume availability” signal across core state, UI components, and skins to prevent showing non-functional volume controls/popovers on platforms (notably iOS Safari) where programmatic volume changes are unsupported.
Changes:
- Introduces async runtime probing of volume support and exposes it via
volumeAvailability(available/unavailable/unsupported). - Propagates volume availability to UI state/data attributes for volume slider and mute button, and gates volume popovers in React/HTML skins.
- Adds/updates tests and documentation for availability detection and popover gating.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/content/docs/reference/volume-slider.mdx | Documents data-availability and styling patterns for unsupported volume control. |
| site/src/content/docs/reference/feature-volume.mdx | Updates examples to gate UI rendering based on volumeAvailability. |
| packages/utils/src/dom/tests/slotted.test.ts | Tightens typing for getSlottedElement usage. |
| packages/react/src/ui/volume-slider/volume-slider-root.tsx | Switches to VolumeSliderDataAttrs so availability is reflected on the root. |
| packages/react/src/ui/volume-slider/tests/volume-slider.test.tsx | Adds tests asserting data-availability and render-state exposure. |
| packages/react/src/presets/video/skin.tsx | Gates the volume popover based on volumeAvailability. |
| packages/react/src/presets/video/skin.tailwind.tsx | Same gating for tailwind video skin. |
| packages/react/src/presets/video/minimal-skin.tsx | Same gating for minimal video skin. |
| packages/react/src/presets/video/minimal-skin.tailwind.tsx | Same gating for tailwind minimal video skin. |
| packages/react/src/presets/audio/skin.tsx | Same gating for audio skin. |
| packages/react/src/presets/audio/skin.tailwind.tsx | Same gating for tailwind audio skin. |
| packages/react/src/presets/audio/minimal-skin.tsx | Same gating for minimal audio skin. |
| packages/react/src/presets/audio/minimal-skin.tailwind.tsx | Same gating for tailwind minimal audio skin. |
| packages/html/src/ui/volume-slider/volume-slider-element.ts | Uses VolumeSliderDataAttrs to apply data-availability. |
| packages/html/src/ui/popover/popover-element.ts | Adds trigger availability gating and mutation observation to rebind/hide popovers. |
| packages/html/src/ui/popover/tests/popover-element.test.ts | Adds coverage for popover binding/hiding based on trigger availability. |
| packages/core/src/dom/store/features/volume.ts | Replaces sync detection with async probe; initializes volumeAvailability to unavailable. |
| packages/core/src/dom/store/features/tests/volume.test.ts | Adds coverage for initial unavailable and probe outcomes. |
| packages/core/src/core/ui/volume-slider/volume-slider-data-attrs.ts | Adds availability -> data-availability mapping. |
| packages/core/src/core/ui/volume-slider/volume-slider-core.ts | Projects volumeAvailability into slider state as availability. |
| packages/core/src/core/ui/volume-slider/tests/volume-slider-core.test.ts | Adds assertions for availability projection. |
| packages/core/src/core/ui/mute-button/mute-button-data-attrs.ts | Adds availability -> data-availability mapping. |
| packages/core/src/core/ui/mute-button/mute-button-core.ts | Adds availability to mute button state. |
| packages/core/src/core/ui/mute-button/tests/mute-button-core.test.ts | Updates state factory/assertions for new availability field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+264
to
+273
| if (node instanceof HTMLElement && this.#isLinkedTrigger(node)) return true; | ||
| } | ||
|
|
||
| for (const node of record.removedNodes) { | ||
| if (node instanceof HTMLElement && this.#isLinkedTrigger(node)) return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
Comment on lines
+140
to
+143
| it('sets data-availability from the volume feature', () => { | ||
| const { Wrapper } = createPlayerWrapper(); | ||
| mockVolumeState.volumeAvailability = 'unsupported'; | ||
|
|
Comment on lines
+156
to
+159
| it('exposes availability to render state', () => { | ||
| const { Wrapper } = createPlayerWrapper(); | ||
| mockVolumeState.volumeAvailability = 'unsupported'; | ||
|
|
Comment on lines
+264
to
+273
| if (node instanceof HTMLElement && this.#isLinkedTrigger(node)) return true; | ||
| } | ||
|
|
||
| for (const node of record.removedNodes) { | ||
| if (node instanceof HTMLElement && this.#isLinkedTrigger(node)) return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
Comment on lines
+48
to
52
| detectVolumeAvailability().then((volumeAvailability) => { | ||
| if (signal.aborted) return; | ||
| set({ volumeAvailability }); | ||
| }); | ||
|
|
|
|
||
| Controls the media volume level. The slider maps its 0–100 internal range to the media's 0–1 volume scale. When the media is muted, the fill level drops to 0 regardless of the stored volume value. | ||
|
|
||
| The root also reflects volume control availability with `data-availability`. On platforms where volume cannot be changed programmatically, such as iOS, the slider exposes `data-availability="unsupported"`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Closes #961