feat(skin): improved responsive design - #1832
Conversation
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
cursor review |
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (10)
Players (5)
Skins (30)
UI Components (39)
Sizes are marginal over the root entry point. ⚛️ @videojs/react
Presets (7)
Media (9)
Skins (27)
UI Components (33)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (72)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (12)
📦 @videojs/media — no changesEntries (14)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretJS sizes are initial static graph totals (minified + brotli). Lazy dynamic chunks are shown separately when present.
Run |
a9b097d to
86a933c
Compare
86a933c to
d3cff73
Compare
d3cff73 to
f7d72c8
Compare
decepulis
left a comment
There was a problem hiding this comment.
Generally these interactions feel good!
- Are variable names like
--scaleor--sizetoo generic? Should they be--media-*scoped? Or are you using those in a limited way I missed? - I don't think there's anything inherently wrong with duplicating items in the DOM, but it does make me wonder a bit about focus management. Like, if we hit fullscreen on mobile and the frame expands, will focus properly be transferred to a non-hidden control?
- I threw Fable and Sol at this and they found a few things that I think are worth addressing:
- The Default Vanilla CSS selector for hiding the trailing time also hides the slider-preview value below 10rem.
- Invalid values passed through
resolveCSSLength()can now resolve to the containing width instead of safely falling back to zero.- The Minimal CSS selector for the dragging fill of a vertical slider cannot match. This is latent in the built-in presets because Minimal now uses horizontal volume sliders, but it affects ejected/custom layouts.
- The Vanilla controls still translate under
prefers-reduced-motion
I'm going to address it in an upcoming PR but I think we leave
Ideally I wouldn't duplicate the elements but how complex do we want the CSS is the issue. If we have user specified functions in CSS, it'd be fairly easy but otherwise I'll find myself duplicating a lot of CSS. Focus management is a good point though.
Sure, I'll take a look. I thought I'd fixed at least one of those but maybe it was fixed in Tailwind only. |
f7d72c8 to
18832af
Compare
18832af to
863eb8f
Compare
863eb8f to
1bf1247
Compare
1bf1247 to
225ad12
Compare
225ad12 to
5d26026
Compare
5d26026 to
4cc769f
Compare
4cc769f to
c9591d8
Compare
c9591d8 to
e431bb6
Compare
e431bb6 to
e7f5f4e
Compare
e7f5f4e to
9244c4d
Compare
9244c4d to
385165f
Compare
385165f to
6f2509d
Compare
6f2509d to
0a20113
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 0a20113. Configure here.
0a20113 to
0baabae
Compare
mihar-22
left a comment
There was a problem hiding this comment.
Love that this PR is trying to clean up and better generalize some of the design system tokens/vars. Helps with the future eject story and tailwind config. I like that the design is opening up some more space and breathing room in the lower controls on small screens. I think there's more work we can do on the overall mobile experience over time but this is great.

Closes #1382
Other changes
Note
Medium Risk
Broad, user-visible control layout changes across HTML and React presets; regression risk is mainly visual/UX and tooltip registration, not security or data.
Overview
Reworks Default and Minimal player chrome (HTML skins, React presets, and live variants) for tighter small screens and clearer fullscreen scaling.
Default video splits the bar into
primaryControls(play, volume, scrubber, captions, settings) andsecondaryControls(cast, AirPlay, PiP, fullscreen). Legacy CSS skins use matchingmedia-controls--primary/--secondarywrappers. Seek ±10s controls are removed from default and minimal video so a standalone captions toggle can sit on the bar; captions stay in the settings menu as well.Volume moves beside play on minimal video/live (popover
side="right", horizontal slider in minimal). Minimal live volume sits in the start button group; audio minimal puts volume before playback rate.Tailwind templates drop ad‑hoc
growand BEM hooks (media-menu__item--submenu,media-sr-only, etc.) in favor of skin tokens likespacerand shared menu classes. Tooltip custom elements register via newdefineTooltip()on all relevant UI bundles (fixing missing defines on live HTML skins). The css-to-tailwind skill doc adds--spacing/--spacing()guidance and a rule against legacymedia-*marker classes. Visual e2e reads thumbnail sizing from--max-heightinstead of the old custom property name.Reviewed by Cursor Bugbot for commit 0baabae. Bugbot is set up for automated code reviews on this repo. Configure here.