fix(icons): use icon exports in ejected skins#1489
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 (8)
Players (3)
Skins (29)
UI Components (25)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (7)
Skins (26)
UI Components (20)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (9)
🏷️ @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 |
53db300 to
be141a1
Compare
There was a problem hiding this comment.
Awesome, few missing pieces:
- The
media-iconelement that's built would need to support lazy loading of families/collections. It currently just loads all icons for both default/minimal. Screenshot below. - I'd probably remove loading icons from the CDN directly for now. We can figure this out at a later time when we're more confident with how we're packaging/shipping them.
be141a1 to
866e3d4
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 866e3d4. Configure here.
866e3d4 to
1a78d5a
Compare
1a78d5a to
359308b
Compare
359308b to
dd127a7
Compare
dd127a7 to
4aa342e
Compare
mihar-22
left a comment
There was a problem hiding this comment.
That all seems to look good to me! I guess only question which we briefly touched on in Slack is whether we want to use the media-icon in the HTML skins to be consistent with eject (not a blocker).
Happy to do that if we prefer? I can do it in a follow-up PR. |


Summary
@videojs/html/icons,@videojs/html/icons/*,@videojs/react/icons, and@videojs/react/icons/*so generated/ejected code no longer imports from private@videojs/icons/*paths.<media-icon>elements instead of inline SVG markup, while React ejected skins keep component icon imports from public@videojs/react/iconspaths.@videojs/html/iconsis export-only, and@videojs/html/icons/elementis the side-effectful custom-element registration path.@videojs/iconselement build to support lazy family loading through loaders, pending-load de-duping, instance re-rendering, and SSR-safe custom-element registration guards.@/iconsimports and rewrites those imports to public@videojs/react/iconspaths when ejected snippets are generated.moduleSideEffectssodefine/*modules andicons/(dist/)?element/*registration modules are preserved while plain icon string exports remain tree-shakeable.@resvg/resvg-js, which preserves the existing server-only OG image route behavior during this workspace update.Closes #1478
Validation
pnpm -F @videojs/icons buildpnpm -F @videojs/html test src/icons/tests/index.test.tspnpm -F @videojs/html buildpnpm -F @videojs/html build:cdnpnpm -F @videojs/react buildpnpm -F site ejected-skinspnpm typecheckpnpm check:workspacepnpm lint:fix:file <changed files>git diff --checkNote
Cursor Bugbot is generating a summary for commit 4aa342e. Configure here.