Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
❌ Deploy Preview for vjs10-site failed. Why did it fail? →
|
📦 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 |
There was a problem hiding this comment.
Pull request overview
This PR expands the documentation around skin styling and customization, and links installation guidance to the new styling docs, addressing the styling documentation gap from issue #674.
Changes:
- Add CSP guidance to the installation docs and link readers to the skins styling section.
- Expand “Customize skins” with basic CSS custom-property customization guidance and clarify the “ejecting” workflow.
- Add a new “Styling” section to the Skins concept doc, covering vanilla CSS vs Tailwind and current limitations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| site/src/content/docs/how-to/installation.mdx | Adds CSP notes and links installation readers to skins styling documentation. |
| site/src/content/docs/how-to/customize-skins.mdx | Adds basic customization via CSS variables and refines the ejecting instructions/examples. |
| site/src/content/docs/concepts/skins.mdx | Adds a styling section (vanilla CSS vs Tailwind), limitations, and simplifies ejected examples via EjectedSkin. |
💡 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.
| - `style-src 'unsafe-inline'` is currently required for some player UI and HTML player styling behavior. | ||
|
|
||
| ### Example | ||
|
|
||
| ```http | ||
| Content-Security-Policy: | ||
| script-src 'self'; | ||
| style-src 'self' 'unsafe-inline'; | ||
| img-src 'self' https: data: blob:; | ||
| media-src 'self' https: blob:; | ||
| connect-src 'self' https:; | ||
| worker-src 'self' blob:; | ||
| ``` |
sampotts
force-pushed
the
docs/skin-styling
branch
from
e5e0780 to
2d490cc
Compare
| ## CSP | ||
|
|
||
| If your application uses a Content Security Policy, you may need to allow additional sources for player features to work correctly. | ||
|
|
||
| ### Common requirements | ||
|
|
||
| - `media-src` must allow your media URLs. | ||
| - `img-src` must allow any poster or thumbnail image URLs. | ||
| - `connect-src` must allow HLS manifests, playlists, captions, and segment requests when using HLS playback. | ||
| - `media-src blob:` is required when using the HLS player variants, which use MSE-backed playback. | ||
| - `worker-src blob:` is required when using the `hls.js` player variants. | ||
| - `style-src 'unsafe-inline'` is currently required for some player UI and HTML player styling behavior. | ||
|
|
||
| ### Example | ||
|
|
||
| ```http | ||
| Content-Security-Policy: | ||
| script-src 'self'; | ||
| style-src 'self' 'unsafe-inline'; | ||
| img-src 'self' https: data: blob:; | ||
| media-src 'self' https: blob:; | ||
| connect-src 'self' https:; | ||
| worker-src 'self' blob:; | ||
| ``` |
There was a problem hiding this comment.
suggestion(non-blocking): installation is a very tight guide. We should consider moving this CSP section to a new security guide (#966) and linking out.
There was a problem hiding this comment.
Sure, we can add some more details there about unsafe-inline if we continue with inline styles. I have an RFC in the works for moving to scoped <style> blocks instead.
| ## Styling | ||
|
|
||
| See the [skins styling](../concepts/skins#styling) section. |
There was a problem hiding this comment.
polish(non-blocking): consider using DocsLinkCard here, like we do in overview.
Maybe, combined with my security comment up above, this section could be something like
## See also
<DocsLinkCard slug="concepts/skins" anchor="styling" description="Some skins expose CSS custom properties">Skins</DocsLinkCard>
<DocsLinkCard slug="concepts/skins" anchor="styling" description="If you have a content security policy (CSP), you may need to enable additional sources">Security</DocsLinkCard>This would require adding an anchor prop to DocsLinkCard and the underlying DocsLink.
There was a problem hiding this comment.
I'll leave out the security page there as I think we need to talk through nonce and unsafe-inline stuff first.
| import DocsLinkCard from '@/components/docs/DocsLinkCard.astro'; | ||
| import Aside from '@/components/Aside.astro'; | ||
|
|
||
| import EjectedSkin from '@/components/docs/EjectedSkin.astro'; |
| | Property Name | Description | Type | Example | | ||
| | ------------- | ----------- | ---- | ------- | | ||
| | `--media-border-radius` | The border radius of the media player | A valid [border-radius value](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/border-radius#values) | `1rem` | | ||
| | `--media-color-primary` | The color of icons and text in media controls | [`<color>`](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value) | `red` | |
There was a problem hiding this comment.
thought: is every skin going to support every CSS custom property? Maybe not now, but we should consider an api reference for each skin. (I know, eventually this will lead to a bloated sidebar, but, one problem at a time 😅 maybe eventually we'll add a gallery or something)
There was a problem hiding this comment.
Sure, can break it out into another page but I'll do that in a follow up PR if that's ok.
| #### Tailwind | ||
|
|
||
| - Currently we're assuming you're using the default configuration and that's all that's supported. With the release of our CLI, this will change, allowing you to specify a custom prefix to the Tailwind classnames. For now, you'll need to edit the ejected skins yourself. | ||
| - We're assuming the latest version, currently 4.2.x. |
There was a problem hiding this comment.
issue(blocking):
you might've noticed that I haven't actually put the ejected tailwind skins in docs yet. Maybe now's the time we add them to customize-skins. I felt blocked by #840, but maybe I was letting perfect getting in the way of good enough.
We should either
- add the tailwind skins to the eject docs or
- remove this paragraph until we do add tailwind to the docs
There was a problem hiding this comment.
I'll go with 2 for now.
| #### Vanilla CSS | ||
|
|
||
| - We use a [BEM](https://en.bem.info/methodology/quick-start/) classname structure and every component classname is scoped with a `media-` prefix. |
There was a problem hiding this comment.
question:
Is this relevant if our skins use shadow dom, unless you eject?
Or do our skins not use shadow dom?
Or do only our html skins use shadow dom? In which case, this info should be <FrameworkCase frameworks=["react"]> scoped.
I'm still so new to the html custom element world I'm not sure if my question makes sense. Lmk if I can clarify!
There was a problem hiding this comment.
Good point. Non-ejected HTML skins don't need a stylesheet since it's in the Shadow DOM but ejected doesn't use the Shadow DOM but it reminds me we should document the stuff we're doing in SkinMixin since it injects a base.css file that sets some required styles. I'll look into that as part of #928 .
There was a problem hiding this comment.
thought: I'm feeling some tension about both concepts/skins and how-to/customize-skins existing.
I actually agree with your choices to put a high-level overview in the concept guide, and then to get into the mechanics in the how-to guide. I'm not actually tossing out any blocking change requests. But I do want to think this through because I fear that the line between the two guides isn't very clear. Like, I can imagine your CSS custom property table existing in either guide. I can even imagine the eject instructions existing in the concept guide, especially once they become a one-liner CLI command. So why did they get their own how-to guide?
What would this look like if we drew the lines in different places? We could...
- leave things as you have them in this PR. There's a tiny bit duplicate content, with the skins concept focusing more on the high-level and customize-skins focusing more on implementation
- break up customize-skins into more specific guides. Call this one how-to/eject-skins and put the CSS custom properties in skins/concepts or an API reference for each skin
- consolidate everything into concepts/skins
Let's have this conversation in person. But also, don't let this conversation be blocking. Just something we should clear out eventually. It's not a problem yet.
There was a problem hiding this comment.
I'm happy to do option 2 if that makes sense. I'll do it in a follow up PR.
sampotts
force-pushed
the
docs/skin-styling
branch
from
2d490cc to
3701f82
Compare
Summary
Testing
Closes #674