docs(site): add ejected skin section to installation guide#1524
docs(site): add ejected skin section to installation guide#1524ronald-urbina wants to merge 11 commits into
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 — no changesPresets (7)
Media (9)
Players (5)
Skins (30)
UI Components (37)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (8)
Skins (27)
UI Components (31)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (11)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretAll sizes are standalone totals (minified + brotli).
Run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
In addition to the comments I made below, it's important for this guide and npx @videojs/cli docs how-to/installation to stay in sync. You should port the changes you made here to that guide as well, so that when agentic readers hit this guide with --skin minimal, they're served the minimal ejected skin as well.
(Though before you spend too much time on that, I'm also starting to wonder if we should link to the eject guide instead of putting eject instructions here... I asked heff about that here. Sorry for bringing up a design conversation like this up after implementation has already started!)
Update: comment below
| } | ||
|
|
||
| export default function EjectedSkinToggleClient({ children }: Props) { | ||
| const [variant, setVariant] = useState<Variant>('default'); |
There was a problem hiding this comment.
imo these should listen to the same nanostore that powers the skin toggle higher up the page.
If you select minimal at the top, you should be viewing the minimal code example at the bottom.
Not relevant in light of the comment below
|
@ronald-urbina we talked about this in a meeting today. We're imagining a slightly different shape for this flow. |
decepulis
left a comment
decepulis
left a comment
There was a problem hiding this comment.
Still some work to do!
- a change to the UI, putting ejected skins in the existing header hierarchy and code blocks rather than replacing it
- some broken tests and typechecking
- some suggestions about refactoring; more logic should live in codegen.ts
| --media <html5-video|html5-audio|hls|background-video> | ||
| --install-method <cdn|npm|pnpm|yarn|bun>`; | ||
| --install-method <cdn|npm|pnpm|yarn|bun> | ||
| --embed-method <packaged|ejected>`; |
There was a problem hiding this comment.
What happens if a user does --embed-method ejected --media background-video? Do we error invalid combos?
There was a problem hiding this comment.
is ignored as it happens if skin is added into a background-video command
There was a problem hiding this comment.
I'd prefer to have an explicit error to help users understand what's going on!
| @@ -1,3 +1,4 @@ | |||
| import ejectedSkins from '@/content/ejected-skins.json'; | |||
There was a problem hiding this comment.
Why does the CLI need to import the generated ejected-skins.json directly? The existing installation CLI path treats packages/cli/src/utils/format.ts as a thin markdown wrapper around the shared site installation generators in site/src/utils/installation/codegen.ts.
This adds a second path for installation codegen where the CLI knows about generated content IDs, the JSON shape, and CSS-style filtering. Could we move the ejected-skin lookup/generation behind the shared installation codegen boundary instead, so the CLI remains a wrapper and the site/CLI do not diverge on how ejected snippets are selected?
This will probably help clear up the typecheck/test errors we're seeing, too.
| --install-method <cdn|npm|pnpm|yarn|bun> | ||
| --install-method <cdn|npm|pnpm|yarn|bun> \ | ||
| --embed-method <packaged|ejected> |
There was a problem hiding this comment.
Once you merge in main, you may have to make a small change to this section here
| </TabsRoot> | ||
| </FrameworkCase> | ||
|
|
||
| <EmbedMethodSection> |
There was a problem hiding this comment.
Hmm I'd like to propose a pretty big UX change.
- Currently, when you select "Eject", the "create your player [react]" and "use your player" headers disappear. We should bring them back.
- Currently, when you select "Eject"...
- in React, the ejected skin currently replaces "create your player" and "use your player". Instead of that, it might be neater if Skin.tsx and skin.css were additional files in "create your player". So, keep the current
./components/player/index.tsx, but add./components/player/skin.tsxand./components/player/skin.css. This will require a rethink of howcodegen.ts > generateReactCreateCodeworks, so it returns multiple files and the front-end shows those multiple files (tabs) and the cli shows those multiple files one-after-the-other
- in HTML, this could be more simple. Just take the ejected skin you have, and replace the code block under "use your player". Again, the logic should live in codegen.ts, not in format.ts
- in React, the ejected skin currently replaces "create your player" and "use your player". Instead of that, it might be neater if Skin.tsx and skin.css were additional files in "create your player". So, keep the current
…und-video inference
There was a problem hiding this comment.
This is a lot closer! Thanks for addressing my feedback.
Here's a few more items for this round 🙂
In addition to my one review comment... two more things!
General comment
Make sure your tests pass before the next round of review!
UX improvement
This is a bit of an awkward experience.
- We should use filenames for all three.
index.html,skin.css, and maybeindex.ts. - They should be in the same tab container, not two separate ones, at this point. If possible, we should only combine them into the same tab container in the ejected case. In the packaged case, it still makes sense to have them as two separate code blocks
let me know if I can clarify!
There was a problem hiding this comment.
I'm worried the string replacement approach you're taking here is brittle. If the generated code changes too much, then these utils will change, too. I'd sugest instead maybe making build-ejected-skins output some sort of parameterizable output. Stable strings like {{ SRC }} that can be easily replaced wherever it's needed -- EjectedSkin.astro and codegen -- independently
There was a problem hiding this comment.
Already done, now the build script outputs stable placeholder tokens and the consumers just swap them in. No more brittle regex.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5dce5fa. Configure here.
| function resolveEjectedPlaceholders(html: string, renderer: Renderer, url: string): string { | ||
| const tag = renderer === 'hls' ? 'hls-video' : renderer === 'html5-audio' ? 'audio' : 'video'; | ||
| return html.replaceAll('{{MEDIA_TAG}}', tag).replaceAll('{{SRC}}', url); | ||
| } |
There was a problem hiding this comment.
Unreplaced poster placeholder
Medium Severity
Ejected HTML generation resolves {{SRC}} and {{MEDIA_TAG}} but not {{POSTER}}, so install examples and the site code block can still show a literal {{POSTER}} in poster img tags after the build script switched to that placeholder.
Reviewed by Cursor Bugbot for commit 5dce5fa. Configure here.
| return html | ||
| .replaceAll('{{SRC}}', VJS10_DEMO_VIDEO.mp4) | ||
| .replaceAll('{{POSTER}}', VJS10_DEMO_VIDEO.poster) | ||
| .replaceAll('{{MEDIA_TAG}}', 'video'); |
There was a problem hiding this comment.
Audio skins show video tag
Medium Severity
resolveHtml always substitutes {{MEDIA_TAG}} with video, so HTML ejected skins for audio (default-audio, minimal-audio) display the wrong media element in the customize-skins and concepts docs.
Reviewed by Cursor Bugbot for commit 5dce5fa. Configure here.
|
|
||
| const isEjected = $embedMethod === 'ejected'; | ||
|
|
||
| if (isEjected) { |
There was a problem hiding this comment.
Ejected UI ignores background preset
Low Severity
The ejected tab layout keys only on embedMethod === 'ejected', not on use case. After choosing ejected then switching to background video, the picker hides but the block still uses the ejected layout while codegen returns packaged HTML without ejected CSS.
Reviewed by Cursor Bugbot for commit 5dce5fa. Configure here.
2 participants
Closes 1385
Adds a new "Ejecting a skin" section to the installation guide so users
can access the ejected skin workflow without going back to the homepage.
Changes
EjectedSkinTogglecomponent with a default/minimal radio switcherthat shows the correct ejected-skin install steps per variant
installation.mdxwired up with the toggle and a teaser link near theskin picker section
Note
Medium Risk
Touches shared installation codegen used by the site and CLI bundle; incorrect ejected JSON or placeholder resolution would break copy-paste install paths, but behavior is heavily tested and background-video is explicitly guarded.
Overview
Adds packaged vs ejected as a first-class installation choice so generated snippets either use pre-built skins or copy ejected skin source into the project.
The installation guide gets an embed method picker (hidden for background-video), and HTML/React code blocks switch to multi-file output (
index.html/index.ts/skin.cssorMyPlayer.tsx+skin.css) when ejected is selected. Shared codegen incodegen.tsbranches onembedMethod, loading snippets via newejected.tsfromejected-skins.json; background-video always stays packaged.The ejected-skins build now emits placeholder HTML (
{{SRC}},{{POSTER}},{{MEDIA_TAG}}) with separatecdnScript/cdnStylesheetfields instead of baking CDN tags into the template. DocsEjectedSkin.astroresolves placeholders for display.@videojs/cli docs how-to/installationgains--embed-method, interactive prompting for embed method, and inference of background-video from--mediawithout extra prompts. CLI bundles aliasejected.tsand formats ejected output consistently with the site.Reviewed by Cursor Bugbot for commit 5dce5fa. Bugbot is set up for automated code reviews on this repo. Configure here.