feat(html): refactor attach contexts to carry state and setter#1024
Merged
feat(html): refactor attach contexts to carry state and setter#1024
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. |
mihar-22
changed the title
Contributor
📦 Bundle Size Report🎨 @videojs/html — no changesPresets (7)
Media (5)
Players (3)
Skins (16)
UI Components (21)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (4)
Skins (14)
UI Components (18)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (6)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (3)
ℹ️ How to interpretAll sizes are standalone totals (minified + brotli).
Run |
Contributor
CI Failure Diagnosis
|
mihar-22
force-pushed
the
claude/jovial-lichterman
branch
from
50959a3 to
396f2c8
Compare
Refactors the HTML custom element context system from simple setter callbacks (`mediaAttachContext`, `containerAttachContext`) to richer context objects that carry both the current value and its setter (`mediaContext`, `containerContext`). This enables descendant elements to read the current media/container reference without needing a separate context. On the React side, adds `container` to `PlayerContextValue` and exposes a `useContainer()` hook for accessing the container element. Extracted from #805. Co-authored-by: Wesley Luyten <luwes@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mihar-22
force-pushed
the
claude/jovial-lichterman
branch
from
396f2c8 to
f6d178d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the HTML custom element “attach” contexts to provide richer media/container context objects (current value + setter), and extends the React player context to expose the current container element via a new useContainer() hook.
Changes:
- HTML: Replace
mediaAttachContext/containerAttachContext(setter-only) withmediaContext/containerContext(value + setter) and update mixins/providers to publish/consume them. - React: Add
containertoPlayerContextValue, plumb it through the Provider, and export a newuseContainer()hook. - Tests: Update React mocks/tests to include
containerand add coverage foruseContainer().
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/testing/mocks.tsx | Updates test wrapper context value shape to include container. |
| packages/react/src/player/tests/context.test.tsx | Adds tests for useContainer() and updates context value helper. |
| packages/react/src/player/create-player.tsx | Passes container through the Provider context value. |
| packages/react/src/player/context.tsx | Adds container to context, introduces useContainer(), and exports it. |
| packages/react/src/media/tests/video.test.tsx | Updates context values used by Video tests to include container. |
| packages/react/src/media/tests/audio.test.tsx | Updates context values used by Audio tests to include container. |
| packages/react/src/index.ts | Re-exports useContainer. |
| packages/html/src/store/provider-mixin.ts | Provider now publishes media/container context objects (value + setter). |
| packages/html/src/store/media-attach-mixin.ts | Consumes mediaContext and uses value.setMedia instead of a bare callback. |
| packages/html/src/store/container-mixin.ts | Consumes containerContext and uses value.setContainer instead of a bare callback. |
| packages/html/src/player/create-player.ts | Wires new mediaContext / containerContext into mixins. |
| packages/html/src/player/context.ts | Introduces mediaContext / containerContext and switches keys to Symbol.for. |
| packages/html/src/media/container-element.ts | Updates container element to use the new containerContext. |
💡 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.
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
mediaAttachContext,containerAttachContext) to richer context objects carrying both current value and setter (mediaContext,containerContext)containerto ReactPlayerContextValueand exposesuseContainer()hookExtracted from #805 to land independently — #805 will rebase on top.
Test plan
pnpm typecheckpassespnpm -F @videojs/html test— 86 tests passpnpm -F @videojs/react test— 195 tests pass (includes newuseContainertests)pnpm build:packagessucceeds🤖 Generated with Claude Code
Co-authored-by: Wesley Luyten @luwes