feat(packages): make <media-poster> act like <picture> - #2039
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 Bundle Size Report🎨 @videojs/html
Small changes (19, ≤ 300 B)
Presets (7)
Media (13)
Players (5)
Skins (30)
UI Components (39)
⚛️ @videojs/react — 27 small size changes
Presets (7)
Media (12)
Skins (27)
UI Components (33)
🧩 @videojs/core — 1 small size change
Entries (76)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (13)
📦 @videojs/jsx — no changesEntries (3)
📦 @videojs/media — no changesEntries (15)
📦 @videojs/spf — no changesEntries (7)
ℹ️ How to interpretEach entry is independently bundled, minified, and brotli-compressed. Initial size includes its static import graph; lazy dynamic chunks are reported separately. Entries are not additive because their dependency graphs overlap. Preset rows represent realistic combined bundles. Changes of 300 B or less across initial, lazy, and total size are collapsed, not discarded. Run |
af96315 to
8ddf995
Compare
8ddf995 to
ece39bf
Compare
decepulis
left a comment
decepulis
left a comment
There was a problem hiding this comment.
A PR review for Claude!
generally, I'm not qualified to say if this PR follows the usual core/ui primitive boundaries. Double check that, too.
| ? `import { Video, VideoSkin, videoFeatures } from '@videojs/react/video';` | ||
| : `import { ${reactMedia.component} } from '${reactMedia.importPath}';\nimport { VideoSkin, videoFeatures } from '@videojs/react/video';`; | ||
|
|
||
| // The poster URL is player configuration, not a skin prop. |
There was a problem hiding this comment.
remove. I don't like comments that talk about historical shape. If people want archaeology, they can read the PR 😅
| /** | ||
| * How far the image the poster coordinates has got. | ||
| * | ||
| * Reported by the binding rather than derived here: only the binding holds the | ||
| * image, and its source may be one the author set rather than the resolved | ||
| * `src` below. `'none'` covers both no image and an image with no source. | ||
| */ |
There was a problem hiding this comment.
imo throughout this whole PR, you've been too verbose with comments. Double check with repo rules and conventions on comments. Consider only using them when they're used in the docs by our api reference generator or particularly needed
| <media-poster class="${poster(true)}"> | ||
| <slot name="poster"></slot> | ||
| <slot name="poster"> | ||
| <!-- Replaced by an <img slot="poster"> of your own. --> |
There was a problem hiding this comment.
don't introduce comments like this unless they were preexisting
| * This is a CSS `background-image` value, not a URL — it is written onto the | ||
| * poster image untouched, so a URL has to arrive wrapped: `url("…")`. Any blur | ||
| * has to be baked into the image itself, which is what `@mux/blurup` returns. | ||
| * Resolved independently of the poster, so a player can carry one without the | ||
| * other. |
There was a problem hiding this comment.
WAY too verbose and specific
| * This is a CSS `background-image` value, not a URL — it is written onto the | |
| * poster image untouched, so a URL has to arrive wrapped: `url("…")`. Any blur | |
| * has to be baked into the image itself, which is what `@mux/blurup` returns. | |
| * Resolved independently of the poster, so a player can carry one without the | |
| * other. |
| The blur has to be part of the image you supply, not something a skin adds. [`@mux/blurup`](https://www.npmjs.com/package/@mux/blurup) returns exactly this: a tiny frame wrapped in an SVG that carries its own Gaussian blur. | ||
|
|
||
| ```ts | ||
| import { createBlurUp } from '@mux/blurup'; | ||
|
|
||
| const { blurDataURL } = await createBlurUp(playbackId); | ||
| ``` |
There was a problem hiding this comment.
I don't want these docs to be mux specific; we'll figure out where to tell the blurup story separately
|
|
||
| The value is a CSS image, not a URL, so wrap a URL in `url()` yourself. Anything CSS accepts works, including a gradient. | ||
|
|
||
| Mind the quoting. A blurred SVG carries double quotes of its own, so `url("...")` around one ends at the first of them and the browser drops the declaration silently — nothing paints, and there is no error. Use `url('...')`, as the examples above do. Written in markup the same value needs its double quotes as `"`, so setting it from script is usually easier. |
There was a problem hiding this comment.
Too specific
| Mind the quoting. A blurred SVG carries double quotes of its own, so `url("...")` around one ends at the first of them and the browser drops the declaration silently — nothing paints, and there is no error. Use `url('...')`, as the examples above do. Written in markup the same value needs its double quotes as `"`, so setting it from script is usually easier. |
| The component fills in a source for you, but you never lose control of the image. `srcset`, `sizes`, `loading`, `<picture>`, and framework image components all stay available. | ||
|
|
||
| <FrameworkCase frameworks={["react"]}> | ||
| The component *is* the `<img>`, so image attributes go straight on it: |
There was a problem hiding this comment.
| The component *is* the `<img>`, so image attributes go straight on it: | |
| The component is the `<img>`, so image attributes go straight on it: |
| </FrameworkCase> | ||
|
|
||
| <FrameworkCase frameworks={["html"]}> | ||
| `<media-poster>` renders no image of its own — it works like `<picture>`, filling in the `src` of the image you put inside it: |
| } | ||
| ``` | ||
|
|
||
| The image is an ordinary child, so style it as a descendant — there is no part to reach through: |
There was a problem hiding this comment.
| The image is an ordinary child, so style it as a descendant — there is no part to reach through: | |
| The image is an ordinary child, so style it as a descendant |
If there's no part to reach through, then why'd you mention it? You need to be way more minimal and efficient with your docs. Again, refer to the skills.
|
Thanks — all of it landed in three commits on top of the four already here, so your inline comments stay attached to the diff they were written against. Squash whenever you like. What changedComments. Cut throughout, and I took your specific suggestions as written. Kept only the class and component JSDoc the reference generator extracts. I also put back four comments I had deleted from the skin tailwind files without meaning to — they were yours, from before this branch.
Tailwind. The Reference page. Gone: the CSS-quoting essay, the One real bug, found on a second pass. The React load handler recorded whatever On core/ui boundariesMostly clean. One thing does cross a line, and I'd like your call rather than my guess:
Also worth naming
Not acted onAn author's image that changes its own
|
decepulis
changed the title
bb57b5a to
c1d5c2b
Compare
|
Absorbed — #1998 has
I left the two JSDoc comments alone, as asked — they still say nothing about URL vs CSS image. The part that matters for your rebaseThe branch is no longer at
Only the imports actually conflicted; the setter body auto-merged, with Verified on the merged branch
Title and body here are updated to cover the placeholder. Generated by Claude Code |
The value reaches CSS untouched, so a URL has to arrive wrapped and any blur has to be baked into the image. That follows from how the poster offers it, so it belongs with the design that does. Refs #1798
`<media-poster>` rendered nothing of its own, so reading the store's resolved poster left no place to put a URL. Owning an `<img>` in a shadow root answers that, and costs three things: the image is stylable only through `::part(img)`, skins carry two styling contracts — one for the internal image, one for the replacement — and two images sit in the tree with one of them hidden. Make it a controller in the shape of `<picture>` instead. It renders no image and coordinates the one already inside it: fills in `src` when the child has none, leaves a child that brought its own `src`, `srcset`, or `<source>` candidates alone, and reports `data-loading`, `data-loaded`, and `data-error` for whichever image is active. The packaged skins carry a plain `<img>` as their poster slot's fallback content, so an `<img slot="poster">` displaces it and there is never a moment when a default and an override both exist. The placeholder needs no component and no second element either. The poster offers the resolved value as `--media-poster-placeholder` — the same property the skins read before this branch, now set by the component that holds the value rather than by `SkinElement` from an attribute. The skins paint it as the poster image's own `background-image`: an `<img>` paints a background behind its content like any other element, so one image covers both jobs and the poster arrives on top. `background-size` tracks `--media-object-fit` so the stand-in occupies the poster's box. Offering the property rather than painting it keeps the component out of presentation: it never overwrites a background the image already had, it costs nothing when no skin reads it, and a skin that wants a separate layer to crossfade can still have one. Custom properties inherit down the flattened tree, so the value reaches the image through both slot hops even when that image lives in another tree. Any blur has to be baked into the image, which is what `@mux/blurup` returns and what Next.js, Angular `NgOptimizedImage`, and Nuxt Image all do. `--media-poster-placeholder-blur` is gone with the CSS filter it fed. Watching an image it does not own means the load treatment now covers one you supplied. It used to apply to the component's own image only and let a supplied one appear abruptly. It also no longer depends on load state: the skins held the image at `opacity: 0` until `data-loaded`, which would now hide the background along with it. `placeholdersrc` on `<video-skin>` and `placeholder` on the React skins are gone, and so is `poster` on the React skins — the URL is player configuration, which is also where the generated e2e pages set it now. `<media-poster>` alone renders nothing, so a hand-authored layout supplies the image and a `__DEV__` warning fires when a poster resolves without one. Refs #1798
The skin templates carry an `<img>` inside their poster slot now, so the transform looking for an empty `<slot name="poster">` stopped matching and left the slot element in output that has no shadow root to compose it. Collapse the slot to the image it carried instead, and give the wrapping `<video-player>` the poster URL, since the image no longer holds one. The React side leaned on `BaseVideoSkinProps` to know a skin took a poster, and that type is gone with the prop. Give the ejected video player its own `poster`, passed to the provider it owns — a standalone player component still needs a way to set one. Refs #1798
Rewrite the poster reference around the image the component coordinates
rather than one it renders: how to supply your own, that it is an ordinary
child with no part to reach through, and what the three load attributes
are for. The placeholder section covers reading
`--media-poster-placeholder`, matching `background-size` to `object-fit` so
the stand-in occupies the poster's box, and the quoting — a blurred SVG
carries double quotes that end a `url("…")` early, and the browser drops
the declaration in silence.
Move the poster URL onto the provider in the demos, the home page, and the
skin examples, and give the HTML demo the image the element now needs.
Update the two poster records in place rather than adding a third: the
compound-image contract and the rejected shadow-root image belong in
`poster.md`, and the placeholder's decisions belong in the record that
already holds them.
Refs #1798
The load handler recorded the `src` it found on the element while the status compared it against the resolved one. An optimizing image component rewrites that attribute, so the two never matched and the poster stayed `data-loading` after the image had loaded — the documented `render` escape hatch. Record the `src` the render asked for instead, which needs no DOM read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
The element carried a derived `#status` field, a write-cache for the placeholder property, and a separate release path that only ever ran on disconnect. Fold those in: the status is a getter, the property is written where it is read, and adopting `null` is what releasing means. 281 lines to 214, with the same behavior and the same tests. Cut the comments back to what is not already in the code, keep the class JSDoc the reference extracts, and drop the abstractions the skin tailwind files grew so those diffs stay readable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
The reference page had grown an essay on CSS quoting, a Mux-specific blur-up walkthrough, and repeated explanations of where the placeholder gets painted. Keep the mechanics, drop the commentary, and link MDN for `<picture>` rather than describing it. Say what a skin cannot do, too: `::slotted(img)` reaches an image you slot directly and nothing inside a `<picture>` you slot, so that sizing is the author's. Both records note it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
The skins used to blur the placeholder themselves on a `::before` layer. Painting it as the poster image's own background removes both that layer and `--media-poster-placeholder-blur`, so name the variable that went and why: a pre-blurred data URL is what image pipelines already return. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
c1d5c2b to
ea749ab
Compare
decepulis
changed the title
<media-poster> act like <picture>
decepulis
left a comment
decepulis
left a comment
There was a problem hiding this comment.
docs could use another pass. a few comments here or there.
| export function getPlaceholderSrc(source: SourceId): string | undefined { | ||
| /** | ||
| * A CSS image. this upscales a 20px thumbnail instead, | ||
| * and the browser's own smoothing does the blurring. |
There was a problem hiding this comment.
is this comment true?
|
|
||
| The poster is visible before playback starts. Once the user plays or seeks, the poster hides permanently — pausing does not bring it back. The poster reappears when a new source is loaded. | ||
|
|
||
| Set the URL on the player, not on this component: |
There was a problem hiding this comment.
This isn't actually accurate, is it? In react, you can set a src directly. in html, you can set a src on a child. Setting it on the player is just one way of doing it. Users should be able to self-sort depending on their use case. "You can set the poster url on the player directly. This is useful if you're trying to change the poster inside a skin, or want to set all your media metadata in one place." Something like that.
| ``` | ||
| </FrameworkCase> | ||
|
|
||
| The value is a CSS image, not a URL, so wrap a URL in `url()` yourself. Anything CSS accepts works, including a gradient, and any blur has to be part of the image. The poster exposes it as the `--media-poster-placeholder` custom property for a skin to paint. |
There was a problem hiding this comment.
| The value is a CSS image, not a URL, so wrap a URL in `url()` yourself. Anything CSS accepts works, including a gradient, and any blur has to be part of the image. The poster exposes it as the `--media-poster-placeholder` custom property for a skin to paint. | |
| Anything CSS accepts works, including a gradient, and any blur has to be part of the image. The poster exposes it as the `--media-poster-placeholder` custom property for a skin to paint. |
|
|
||
| ## Supply your own image | ||
|
|
||
| The component fills in a source for you, but you never lose control of the image. `srcset`, `sizes`, `loading`, [`<picture>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture), and framework image components all stay available. |
There was a problem hiding this comment.
on second thought, don't link to picture
| <Poster srcSet="poster-480.jpg 480w, poster-1080.jpg 1080w" sizes="100vw" /> | ||
| ``` | ||
|
|
||
| To render something else, use `render`. It receives the resolved `src`: |
There was a problem hiding this comment.
God I hate how AI writes. "It receives the resolved src". Why should users know what "resolved" means in this context? Either define it or don't use it. Adhere to the jargon guidance in writing-style.md more diligently; consider the user arriving at this guide who doesn't have the same context you do.
|
|
||
| <FrameworkCase frameworks={["react"]}> | ||
| React renders an `<img>` element. Add a `className` to style it: | ||
| React renders the `<img>` itself. Add a `className` and use it as the selector: |
There was a problem hiding this comment.
| React renders the `<img>` itself. Add a `className` and use it as the selector: | |
| React renders an `<img>` element. Add a `className` to style it: |
Why did you rephrase a perfectly legible sentence and made it more confusing
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
`Poster` filled `src` from the store whenever no `src` prop was passed, even when the caller supplied a `srcSet`. HTML's `hasSource` treats either attribute as the author's, so the two platforms disagreed, and the file disagreed with itself: the load-status branch already counted `srcSet` as a source. It bites with `x` descriptors, where `src` joins the candidate set as the 1x entry: `<Poster srcSet="poster@2x.jpg 2x" />` would load the player's poster on a 1x display. With the `w` descriptors the docs use, `src` is ignored, which is why it went unnoticed. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
"Set the URL on the player, not on this component" was false in both frameworks: React takes a `src` prop and HTML takes one on the child. Say the player is one option and give the reasons to pick it. Cut the jargon a reader arriving cold would trip on. "Resolved" is our word, not theirs, so name the thing instead. Drop the MDN links on `<picture>`, drop the "not a URL" framing the style guide rules out, and tighten the sentences that had two referents for one pronoun. The sandbox comment kept an "instead" whose other half was edited away. Say what the 20px thumbnail is for and stop there. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
|
Two commits: the docs pass, and one real bug Bugbot caught. DocsTook your suggestions on 72 and 164 as written. The rest: Line 44 was wrong, and you were right about why. "Set the URL on the player, not on this component" contradicts the section two headings below it. Now it says the player is one option and gives the reasons to pick it, close to your wording. The same false claim was in the Jargon. "Resolved" is gone from the prose in both places. The accessibility line leaned on it to make a point it could make plainly: a URL says nothing about what the image shows. Also dropped both MDN links on While in there: two sentences had one pronoun doing two jobs, and "hides permanently" was three sentences from "shows it again." One "resolved" survives, in "Is this comment true?"True in substance, not as English. The 20px thumbnail is real, and the upscaling really is what blurs it — no filter anywhere. But "instead" lost its other half when the blurup explanation came out, so it points at nothing. It also ran to two lines for a one-line fact. Now: /** A 20px thumbnail, upscaled by the browser into a blur. */The bugBugbot found a real one and I'd missed it. It bites with There's a test, and I confirmed it fails without the fix. ChecksReact 409, site 531, site build renders both framework variants. Generated by Claude Code |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8408d3a. Configure here.
The outcome was keyed on `src` alone. Now that a `srcSet`-only image keeps `src` empty, swapping the `srcSet` left the key unchanged, so `data-loaded` carried over into the next fetch instead of reporting `data-loading`. Key it on both halves of the source. React sees a prop change here, so this costs nothing; the HTML element still cannot see an author mutate its own image in place, which stays a known limitation. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
2 participants
Closes #1798.
Sandbox
What is this?
<media-poster>fills in the source of the image already inside it, the way<picture>does. As a side effect, it extendsMediaElementrather thanMediaUIElementnow, and drops its lazy, unused i18n chunk.posterPlaceholdercomes from the store and reaches CSS as--media-poster-placeholder, and the skins paint it onimg { background-image: var(--media-poster-placeholder) }.posterandplaceholdersrccome off the skins. Both values come from the player now.The API
Passing to the provider
Consuming in the image
<source>candidates inside a<picture>count as a source too.In React the component is the image:
Overriding the skin image
Painting a poster placeholder
What this isn't
posterAlt. Filling in asrchas<picture>as precedent; describing an image doesn't.media-thumbnailis untouched. Whether it should follow this model is its own question.Funny business
<media-poster>alone renders nothing. Your layout supplies the image, and a__DEV__warning fires when a poster resolves without one.<picture>behaves the same way, though it's a platform element, so expectations differ.::slotted(img)doesn't reach inside a<picture>or a framework image component, and shadow CSS has no other way in. That sizing is yours.setPropertyno-ops on a value it can't parse, so a data URL carrying double quotes needsurl('…').::beforelayer, and--media-poster-placeholder-blurwith it. Recorded as a decision.opacity: 0untildata-loadedwould hide its background too.srcin place keepsdata-loadedthrough the next fetch: we listen forloadanderrorbut don't watch the attribute.🤖 Generated with Claude Code
https://claude.ai/code/session_019fBNWkGLWPzsP4QK6w7u2L
Note
High Risk
Breaking public API change: poster/placeholder leave skin props for the player, and the HTML poster element is rewritten. Skins, CSS blur-up behavior, and consumer markup all need updates.
Overview
Reworks poster so the URL lives on the player, and the component only coordinates the image.
<media-poster>now behaves like<picture>: it fillssrcon a child/slotted image when none is authored, and leavessrc/srcset/<picture>alone otherwise. ReactPosterdoes the same for its rendered<img>. Packaged skins ship a fallback<img>in the poster slot.posterandposterPlaceholdermove off skins onto the provider (Player.Provider/<video-player>). Placeholder is a CSS image (e.g.url('…')), exposed as--media-poster-placeholder. Skins paint it as the image’sbackground-imageinstead of a blurred::beforelayer —--media-poster-placeholder-bluris gone.Also adds
data-loading/data-loaded/data-error, updates sandbox/docs/ejected skins, and drops skinplaceholdersrc/poster/placeholderprops.Reviewed by Cursor Bugbot for commit 73cee0f. Bugbot is set up for automated code reviews on this repo. Configure here.