fix(skins): slightly more idiomatic Tailwind, added custom properties

[sampotts]

• Add --vjs-border-radius to all skins so that border radius can be baked in but controlled a little easier by consumers.
• Add --vjs-font-family to HTML skins so we set a default font family. The assumption being that the Tailwind users would have a font stack set to inherit from. I'm open to discussion on this.
• Make the Tailwind slightly more idiomatic. I think it's hard because we have the nested elements for tooltip content and icons. Perhaps this logic to hide/show should be elsewhere (e.g. via a hook) and not in the CSS. That's probably what I'd expect in React at least.

[vercel]

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	Nov 10, 2025 11:40am
vjs-10-demo-next	Ready	Preview	Comment	Nov 10, 2025 11:40am
vjs-10-demo-react	Ready	Preview	Comment	Nov 10, 2025 11:40am
vjs-10-website	Ready	Preview	Comment	Nov 10, 2025 11:40am

[mihar-22]

Awesome, thank you! Only a few notes in addition to review:

1. Worth a discussion with the team on whether we'll use media- or vjs- prefix. We should be consistent with our custom element prefix.
2. Can we drop styles.ts and move all styles directly into markup? Good step towards being more idiomatic, don't think styles.ts is a recognized pattern in TW-land. I understand we did it for early compiler goals and testing.
3. Can we drop manual addition of vjs: utility prefix? Create a new temporary utility in skin files which calls cn and adds prefix for us. Temporary because we'll have an official vanilla CSS import not generted with TW compiler as it is today. I'm assuming the TW compiler doesn't require this prefix to be statically analyzable when set?

[mihar-22]

I think we should extend the theme to support this value? Something like --radius-chrome which would be vjs:rounded-chrome? Alternative could be brand, theme, or vjs. Similar comment with font family and other config vars.

[sampotts]

I was aiming to avoid having folks extend or change their configuration ideally. It just creates a complexity that's a bit too much for some and then we get support requests asking why it doesn't look right. That said, it works for Shadcn but perhaps that's because it's a wider component library rather than a single component. Probably a discussion point.

[sampotts]

It also raises the question of how the compiler would handle it but perhaps we just have known aspects that we have to handle separately.

[mihar-22]

Ye I think similar to shadcn we would just have these included either manually or codemod when the skin is added via CLI. Worth a discussion for sure. I think if we're building a design system as part of Tailwind then this is something we should do idiomatically and avoid littering vars inline. It's also something the compiler could/should extract from the config into vanilla CSS. Again, love to chat more about it :)

[mihar-22]

Usage of has here is a code smell to me that we need a Overlay (maybe different name, Scrim?) component with it's own data attributes.

[cjpillsbury]

Agree with the general sentiment. If MediaContainer had all properties by default, would that work?

[sampotts]

Yeah, I've anticipated all of this controls hide/show logic this is temporary. Having a dedicated component would be ideal. Backdrop might be another suitable name perhaps (aligns with the native <dialog> backdrop etc).

[mihar-22]

Ye we would need a dedicated component to avoid relying on parent state to style this elmenet which is breaking encapsulation.

[mihar-22]

Similar to overlay, code smell to me that we need a Controls component, which we actually do anyway.

[sampotts]

Agreed. This is temporary logic again.

[mihar-22]

Cool, worth a callout here in terms of not being idiomatic or breaking encapsulation. I wasn't expecting to fix in this PR. I'll have tickets up for these eventually.

[mihar-22]

We should be able to remove these semantic classes now as the compiler won't need them since we discovered the design system loader. Similar comment with others like it in this file.

[sampotts]

Yeah, we can possibly remove them later once we have the compiler but they're currently needed to hide/show the correct content.

For me, the most idiomatic approach (in React, at least) for the tooltip content would be conditional rendering:

{isPlaying ? t('PAUSE') : t('PLAY')}

(assuming we have a t function for i18n at some point).

[mihar-22]

Cool! I imagine few ways we can handle this:

1. Use the render function on the PlayButton to access state and render tooltip content accordingly inside. I don't mind this but it does feel funny with tooltip inside the button.
2. Use a hook like you've mentioned. We can just pluck state from media store or package up a more specific hook so devs don't need to guess which state they need. Imperative approach and more barebones.
3. PlayTooltip component and use data attrs for showing/hiding. Declarative approach and exposes styling hooks.

I like 3 because it's consitent with other tooltips structurally and we can expose additional data attributes for styling. Doesn't preclude us from having the hook as well. It's similar to how we have different versions of "toggle" buttons like play, mute, captions, etc. I originally wasn't a fan but the more I think about it, makes sense.

[sampotts]

Awesome, thank you! Only a few notes in addition to review:

1. Worth a discussion with the team on whether we'll use media- or vjs- prefix. We should be consistent with our custom element prefix.
2. Can we drop styles.ts and move all styles directly into markup? Good step towards being more idiomatic, don't think styles.ts is a recognized pattern in TW-land. I understand we did it for early compiler goals and testing.
3. Can we drop manual addition of vjs: utility prefix? Create a new temporary utility in skin files which calls cn and adds prefix for us. Temporary because we'll have an official vanilla CSS import not generted with TW compiler as it is today. I'm assuming the TW compiler doesn't require this prefix to be statically analyzable when set?

1. Agree, I only used vjs since that's how we're prefixing the classnames and I didn't want to break things. For me, we need to ditch the prefix anyway, that's probably the least idiomatic aspect currently and defeats the object of Tailwind (to reduce CSS size and complexity).
2. I thought a lot about this, but I don't think it's as easy as ditching it and throwing the classNames on the elements directly. We're going to end up with a lot of duplicate classnames (some are shared - e.g. buttons, icons, transitions on popovers and tooltips) which obviously creates a maintenance burden. My thought on it was we'd inline the classNames for an ejected React skin.
3. I tried adding it to the cn function but then the classnames become dynamic which Tailwind's compiler doesn't work with.