refactor(html): context-based media discovery, remove slot="media"#997
refactor(html): context-based media discovery, remove slot="media"#997
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. |
📦 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 |
Skins now use default `<slot>` instead of `<slot name="media">`. Users no longer need `slot="media"` on their media elements — the provider discovers media via context (primary) or querySelector fallback (for plain <video>/<audio>). The internal `<slot name="media">` inside CustomMediaElement (for native element override) is unchanged. Refs #923 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Custom media elements (HlsVideo, SimpleHlsVideo, DashVideo) now self-register with the provider via mediaAttachContext using a new MediaAttachMixin. BackgroundVideo registers its inner <video> directly. Uses the raw context-request protocol so no ReactiveControllerHost is required. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace local CustomElementLike with CustomElement from @videojs/utils/dom - Add getMediaTarget() hook so subclasses control what registers as media - BackgroundVideo now extends MediaAttachMixin(HTMLElement) and overrides getMediaTarget() to return this.target - Remove unnecessary casts and pointless comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The selector targeted [slot="media"] which no longer exists after removing the slot="media" attribute requirement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9803dd3 to
5bb6f4f
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors HTML media discovery to be context-based (with a provider querySelector fallback for plain <video>/<audio>), and updates skins/templates to use a default <slot> so users no longer need slot="media" on media elements.
Changes:
- Replace named
mediaslots in skins/templates with the default slot. - Introduce
MediaAttachMixinso custom media elements register themselves viamediaAttachContext. - Add a decision record documenting the approach.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sandbox/templates/html-video/main.ts | Removes slot="media" from the native <video> example. |
| packages/sandbox/templates/html-simple-hls-video/main.ts | Removes slot="media" from <simple-hls-video> example. |
| packages/sandbox/templates/html-hls-video/main.ts | Removes slot="media" from <hls-video> example. |
| packages/sandbox/templates/html-dash-video/main.ts | Removes slot="media" from <dash-video> example. |
| packages/sandbox/templates/html-background-video/main.ts | Removes slot="media" from <background-video> example. |
| packages/sandbox/templates/html-audio/main.ts | Removes slot="media" from the native <audio> example. |
| packages/html/src/store/media-attach-mixin.ts | Adds a mixin intended to request attach context and register media with the provider. |
| packages/html/src/media/simple-hls-video/index.ts | Applies MediaAttachMixin to SimpleHlsVideo. |
| packages/html/src/media/hls-video/index.ts | Applies MediaAttachMixin to HlsVideo. |
| packages/html/src/media/dash-video/index.ts | Applies MediaAttachMixin to DashVideo. |
| packages/html/src/media/background-video/index.ts | Applies MediaAttachMixin to BackgroundVideo and registers the inner <video> target. |
| packages/html/src/index.ts | Exports media-attach-mixin from the public entrypoint. |
| packages/html/src/define/video/skin.ts | Switches video skin from <slot name="media"> to default <slot>. |
| packages/html/src/define/video/skin.tailwind.ts | Switches Tailwind video skin from named to default slot. |
| packages/html/src/define/video/minimal-skin.ts | Switches minimal video skin from named to default slot. |
| packages/html/src/define/video/minimal-skin.tailwind.ts | Switches Tailwind minimal video skin from named to default slot. |
| packages/html/src/define/background/skin.ts | Switches background skin from named to default slot. |
| packages/html/src/define/background/skin.css | Updates selector from [slot="media"] to targeting background-video. |
| packages/html/src/define/audio/skin.ts | Switches audio skin from <slot name="media"> to default <slot>. |
| packages/html/src/define/audio/skin.tailwind.ts | Switches Tailwind audio skin from named to default slot. |
| packages/html/src/define/audio/minimal-skin.ts | Switches minimal audio skin from named to default slot. |
| packages/html/src/define/audio/minimal-skin.tailwind.ts | Switches Tailwind minimal audio skin from named to default slot. |
| packages/element/src/context.ts | Adjusts @videojs/element/context re-exports to include an event class. |
| internal/decisions/context-media-discovery.md | Adds decision doc explaining context-based discovery and fallback behavior. |
💡 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.
luwes
left a comment
There was a problem hiding this comment.
LGTM!
For skins which do still use real shadow dom manual slot assignment came to mind.
Could that still be useful? Just throwing it out there.
https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/slotAssignment
Remove slot="media" from all docs, demos, and site components after #975 and #997 made it unnecessary. Update prose that described the old MutationObserver/slot-based attachment pattern. Add reference pages for MediaAttachMixin and useContainerAttach. - Remove slot="media" from 22 demo HTML files - Remove slot="media" from homepage and installation code generators - Remove slot="media" from all MDX concept and reference pages - Fix overview.mdx line 119: drop incorrect slot="media" guidance - Rewrite player-container.mdx: media attachment section now correct - Rewrite container-mixin.mdx: drop MutationObserver description, describe actual behavior (registers self via containerAttachContext) - Rewrite provider-mixin.mdx: document store.attach() ownership, mediaAttachContext/containerAttachContext, querySelector fallback - Add reference/media-attach-mixin.mdx (new) - Add reference/use-container-attach.mdx (new) - Add both new pages to docs.config.ts sidebar Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR #997 removed slot="media" from all skin templates. This restores the named slot alongside the new default slot so users who haven't migrated yet don't break. The slot is marked @deprecated with a comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes #923
Summary
<slot>instead of<slot name="media">— users no longer needslot="media"on their media elementsquerySelectorfallback (for plain<video>/<audio>)internal/decisions/context-media-discovery.mdCustomMediaElement's internal<slot name="media">(for native element override) is unchanged