fix: skin syntax usage cleanup by cjpillsbury · Pull Request #48 · videojs/v10

cjpillsbury · 2025-10-09T21:30:34Z

Some refactors to make skin compilation simpler and more reliable. Multiple callouts in self-added PR comments.

vercel · 2025-10-09T21:30:41Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
vjs-10-demo-html	Ready	Preview	Comment	Oct 10, 2025 7:21pm
vjs-10-demo-react	Ready	Preview	Comment	Oct 10, 2025 7:21pm
vjs-10-website	Ready	Preview	Comment	Oct 10, 2025 7:21pm

…nts (WIP).

…cularly on the web component side.

…ponents.

…n main).

…or compiler testing, iteration, and validation.

cjpillsbury · 2025-10-10T19:21:28Z

@@ -0,0 +1,136 @@
+import type { PropsWithChildren } from 'react';


Don't worry about maintaining these or keeping them in sync with skin updates/refactors. Here primarily for compiler iteration, validation, and other testing.

cjpillsbury · 2025-10-10T19:21:55Z

  },
  "dependencies": {
    "@vjs-10/react": "workspace:*",
+    "@vjs-10/react-icons": "workspace:*",


Added to test in-app defined skins.

cjpillsbury · 2025-10-10T19:22:24Z


 // Register the custom element
 if (!globalThis.customElements.get('media-current-time-display')) {
-  // @ts-ignore - Custom element constructor compatibility


Some eslint error strays I caught. Sorry for the noise!

cjpillsbury · 2025-10-10T19:24:22Z


  #setupPortal(): void {
-    const portalId = this.getAttribute('root-id');
+    const portalId = this.getAttribute('root-id') ?? '@default_portal_id';


This and the corresponding changes in react, as well as the related changes to the respective media containers, are very much a stopgap. We need to build a "real architecture" here, and hopefully can use media container as the default owner of (at least tooltip-related) portals to simplify use cases. Done for now to simplify problem space for complier.

cjpillsbury · 2025-10-10T19:26:15Z

  return /* html */ `
    <slot name="media"></slot>
    <slot></slot>
+    <div id="@default_portal_id" style={ position: absolute; zIndex: 10; }>


Dum dum solution for default portals. slot bits were due to styling inheritance between skin-defined tooltip styles vs. container component shadow dom-defined portal root (and css boundaries implicated therein). On @mihar-22's radar as a tough problem that needs to be figured out.

cjpillsbury · 2025-10-10T19:26:49Z

-customElements.define('media-container', MediaContainer);
+// Register the custom element
+if (!globalThis.customElements.get('media-container')) {
+  // @ts-expect-error ts(2345)


We need to carve out time to fix all of our TS woes at some point...

cjpillsbury · 2025-10-10T19:27:43Z

@@ -1,5 +1,4 @@
 import { MediaSkin } from '../media-skin';
-import { uniqueId } from '../utils/element-utils';



simplified skin for compiler

cjpillsbury · 2025-10-10T19:28:46Z

      <div ref={composedRef} {...props}>
        {children}
+        {/* @TODO We need to make sure this is non-brittle longer term (CJP) */}
+        <div id={portalId} style={{ position: 'absolute', zIndex: 10 }} />


same as above convo (though react side required less hackrobatics. Thoughts on the table are using context providers and consumers for some of these hack-ins.

cjpillsbury · 2025-10-10T19:29:49Z

            <PlayButton className={`${styles.Button} ${styles.IconButton} ${styles.PlayButton}`}>
-              <PlayIcon className={styles.PlayIcon}></PlayIcon>
-              <PauseIcon className={styles.PauseIcon}></PauseIcon>
+              <PlayIcon className={`${styles.PlayIcon} ${styles.Icon}`} />


skin simplification for compiler. Used for descendant selectors (previously used svg, which is not appropriately equivalent for html/web component equivalent).

cjpillsbury · 2025-10-10T19:30:22Z

@@ -1,180 +0,0 @@
-/** @TODO: Improve/Polish CSS Here */


Removing to avoid repo noise. It's in history if we ever want/need to refer back.

cjpillsbury · 2025-10-10T19:35:06Z

 import type { MediaDefaultSkinStyles } from './types';

-import { cn } from '../../utils/cn';
+// NOTE: Removing import to sidestep for compiler complexity (CJP)


Skin styles simplification for compiler. TL;DR:

imports would need to be pulled in as well. Longer term we may want to either solve this or just bake in some common css+js(x) utils into the compiler (cn, clsx, maybe others)

Make sure non-tailwind-utility classes correspond with key names.

Avoid selectors that use element names, since they may not be appropriately equivalent when cross-compiling, especially for VJS components (e.g. [&_svg] vs. added Icon: cn('icon') + [&_.icon])

If key names are intended to correspond with component names, try to make them be exact matches (e.g. FullScreen vs. Fullscreen. Previously also VolumeButton vs. MuteButton, TimeSlider vs. TimeRange, etc. etc.)

cjpillsbury · 2025-10-10T19:35:32Z

        <PlayButton className={`${styles.Button} ${styles.IconButton} ${styles.PlayButton}`}>
-          <PlayIcon className={styles.PlayIcon}></PlayIcon>
-          <PauseIcon className={styles.PauseIcon}></PauseIcon>
+          <PlayIcon className={`${styles.PlayIcon} ${styles.Icon}`} />


Added Icon for reasons discussed in styles.ts comments.

cjpillsbury · 2025-10-10T19:58:59Z

@@ -1,12 +1,14 @@
 export { CurrentTimeDisplay } from './components/media-current-time-display.js';


Adding missing component and other relevant exports to main module for compiler efforts.

cjpillsbury · 2025-10-10T19:59:08Z

 export { MuteButton } from './components/MuteButton';
 export { PlayButton } from './components/PlayButton';
 export { Popover } from './components/Popover';
+export { TimeSlider } from './components/TimeSlider';


Adding missing component and other relevant exports to main module for compiler efforts.

vercel bot temporarily deployed to Preview – vjs-10-website October 10, 2025 15:14 Inactive

vercel bot temporarily deployed to Preview – vjs-10-demo-html October 10, 2025 15:14 Inactive

vercel bot temporarily deployed to Preview – vjs-10-demo-react October 10, 2025 15:14 Inactive

cjpillsbury added 9 commits October 10, 2025 10:24

fix(html, react): Cleanup of skins and exports for compiler requireme…

9e0cbdd

…nts (WIP).

fix(react): Continue cleanup for build tooling.

d44b5d0

fix: migrating default tooltip portal to container. Some hacks, parti…

897fb06

…cularly on the web component side.

fix(react, html): cleanup main module exports to include relevant com…

484b3ea

…ponents.

chore: Add react-icons dep to demo app to test external skins.

0c52206

chore: reset pnpm-lock based on react et al. version woes (resolved i…

664ea1e

…n main).

chore: re-updating pnpm-lock for local changes.

6ef0b71

fix(react): More skin cleanup for build tooling.

d299086

chore: Adding skins to example app (commented out from App imports) f…

a919f39

…or compiler testing, iteration, and validation.

cjpillsbury force-pushed the fix/skin-syntax-usage-cleanup branch from e91f51a to a919f39 Compare October 10, 2025 19:20

vercel bot temporarily deployed to Preview – vjs-10-demo-html October 10, 2025 19:20 Inactive

vercel bot temporarily deployed to Preview – vjs-10-website October 10, 2025 19:20 Inactive

vercel bot temporarily deployed to Preview – vjs-10-demo-react October 10, 2025 19:21 Inactive

cjpillsbury commented Oct 10, 2025

View reviewed changes

cjpillsbury marked this pull request as ready for review October 10, 2025 19:36

cjpillsbury commented Oct 10, 2025

View reviewed changes

cjpillsbury merged commit d4f3f4b into main Oct 10, 2025
4 checks passed

github-actions bot mentioned this pull request Oct 24, 2025

chore: release main #130

Merged

mihar-22 deleted the fix/skin-syntax-usage-cleanup branch November 24, 2025 03:50

github-actions bot mentioned this pull request Feb 26, 2026

chore: release main #625

Merged

		@@ -0,0 +1,136 @@
		import type { PropsWithChildren } from 'react';

		@@ -1,5 +1,4 @@
		import { MediaSkin } from '../media-skin';
		import { uniqueId } from '../utils/element-utils';

		@@ -1,12 +1,14 @@
		export { CurrentTimeDisplay } from './components/media-current-time-display.js';

Conversation

cjpillsbury commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cjpillsbury commented Oct 9, 2025 •

edited

Loading

vercel bot commented Oct 9, 2025 •

edited

Loading