feat(studio): the Renders panel on the shared primitives - #3772
Conversation
The four native selects become the shared Select, so no OS popup is left in the panel and the four controls read as the same control as everything around them. Disabled options survive the move: SelectOption gains a disabled flag, which is what keeps a resolution the composition cannot reach out of the keyboard's path while its label still explains why. Export drops its own type size. cn resolved that override by dropping the size recipe's step, which is what left the panel Export a type step below the header's; the computed-style table now reads 28px / 6px / 12px for both. Its two per-row siblings become IconButton with a Tooltip carrying what the title attribute used to say. The panel's colours move to the semantic tokens.
…ot vitest 4's loose default
4fe3390 to
142a68c
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE. Three non-blocking findings below, all in the seam between this panel and the shared primitives rather than in the panel itself.
Most of this diff is provably a no-op, and the theme file is the proof
The panel-* → unprefixed migration is not a re-colouring at all — those tokens are aliases:
--color-panel-input: var(--color-input); /* theme.css:137 */
--color-panel-hover: var(--color-hover); /* :139 */
--color-panel-border: var(--color-border); /* :140 */
--color-panel-text-0…5: var(--color-text-0…5);/* :143-148 */
--color-panel-accent: var(--color-accent); /* :151 */
--color-panel-danger: var(--color-danger); /* :152 */Same for the type scale: --text-step-9/10/11 are 9px/10px/11px (:71-73), so every text-[10px] → text-step-10 is exact. That settles the large majority of 305/181 without opening a capture.
Which makes the genuine deltas a short list, and worth having as the checklist for the capture pass — each is a real pixel change, not an alias:
| site | before | after | direction |
|---|---|---|---|
| error text, failed dot | red-400 oklch(70.4%) |
danger = #ef4444 (red-500) |
deeper red |
| FormatInfoTooltip border | neutral-700 oklch(37.1%) |
border-input #27272a |
darker |
| FormatInfoTooltip divider | neutral-800 oklch(26.9%) |
border #1e1e1e |
darker |
| cancelled dot | neutral-600 oklch(43.9%) |
text-5 #3f3f46 |
darker |
| FormatInfoTooltip | shadow-lg |
shadow-menu |
deliberate |
Also verified every new token resolves — text-0…5, surface, input, hover, border, border-input, accent, danger, shadow-menu are all defined. A token rename that lands on a name the theme doesn't define fails silently, so this was the thing worth checking.
1. The disabled Download's tooltip probably never appears
New copy, RenderQueueItem: label={isComplete ? "Download" : "Available when the render finishes"}, and the comment says disabled: on the shared Button "keeps pointer events alive precisely so a disabled one can still say why."
The mechanism doesn't hold up:
IconButton.tsx:28passes the nativedisabledattribute to the<button>. Browsers suppress pointer events on disabled form controls outright — that suppression is not CSS, so not addingpointer-events-none(buttonBase:41is onlydisabled:opacity-40 disabled:cursor-not-allowed) doesn't buy the hover back.Tooltip.tsx:29renders the trigger as<span className="contents">.display: contentsgenerates no box, so the wrapper has no hit area of its own to catch what the disabled child won't emit. Wrapping a disabled control in a box-generating element is the usual fix precisely becausecontentsisn't one — and the docblock states that as the intent ("so a disabled control still gets hover"), which is the reading I'd expect anyone to take on faith.
So the "why" copy is likely dead, and disabled controls get no focus either, so keyboard users don't reach it another way. Non-blocking: the old code had no title when incomplete either, so nothing regresses — this is a new affordance that may not exist rather than a broken one. And I could not run a browser, so what I can say precisely is that nothing asserts it: no test covers the tooltip on a disabled trigger. If you want it, the change is in Tooltip.tsx (give the trigger a real box, e.g. inline-flex), which is out of scope here.
2. "the keyboard skips it" — your own test says otherwise
refuses a resolution the composition cannot reach arrows down once from the default and hits Enter on a 1280×720 comp, then expects "auto". If the keyboard skipped the disabled 1080p it would have landed on 4K — a 3× integer scale of 1280×720, so enabled — and Enter would have committed landscape-4k. The sibling 4K test arrows two steps and does commit, so the sequence isn't inert.
So the real behaviour looks like the opposite of the comment: the highlight can land on the disabled option and Enter declines, leaving the prior value. Worth fixing the comment, because the next person changing scaleApplies will test against the wrong model.
One rider on that test: its expected value is the default, so that half would also pass if the interaction did nothing. The 4K test is what rules that out — fine as a pair, just not independently.
3. Delete lost its destructive hover
Old: text-panel-text-5 hover:text-red-400. New: IconButton at the default ghost variant, so the red-on-hover cue for the one destructive control in the row is gone. Deliberate design-system convergence is a fine answer; flagging it because it's the only affordance in the diff that changed rather than moved.
Credits worth naming
- AE3 —
expect(classes).toEqual(new Set([...shared.split(/\s+/), "w-full"]))is set equality, notcontains, so it catches the extra-class-wins-the-merge bug by construction. That's the right instrument for a bug aboutcnresolution. - KTD13 — asserting both references are
truebefore comparing trigger to reference. Two falses would have agreed and proven nothing; you closed that explicitly. - Dropping
titlefromQUALITY_OPTIONSloses nothing: the old<option>render only ever emitted{q.label}, so those strings were already dead. localStorage.clear()inbeforeEach— necessary once the controls persist, and easy to omit.
Gate
Checked rather than taken on faith: CODEOWNERS covers only the three capture scripts, none of the four changed files; all five commits are attributed to miguel-heygen; no other reviews exist and reviewDecision was REVIEW_REQUIRED. So one approval does clear it, as you said.
— Rames
jrusso1020
left a comment
There was a problem hiding this comment.
Re-stamp at 51538421. The "content unchanged from 16b6c828" claim holds at the strongest level I can check, so my earlier review still describes exactly what merges.
Verified, one pass:
- Blob SHAs identical for all four files at both heads —
RenderQueue.test.tsx7346628b,RenderQueue.tsx3a404838,RenderQueueItem.tsx72dab352,Select.tsx0c7acf67. The bytes I read are the bytes shipping. - File set identical — same four, nothing added or dropped under the stamp.
- Counts unchanged —
133+/25-,99+/92-,69+/64-,4+/0-. - The base move brought nothing into this PR's surface.
51538421is a merge of16b6c828with main atac503920. Comparing the old merge base9bec64a2againstac503920across all 41 files incomponents/ui/andcomponents/renders/: every blob identical. So main touched neither the files this PR edits nor the shared primitives it consumes, and the merge could not have reshaped the change.
Approving on code merit, as before. I did not use CI as a basis — for the record it is still running at this head.
— Rames
Lands unit U13 (Renders panel sweep) of the Studio design-system foundation. Stacked on the value-controls PR (#3626). Hex ratchet unchanged: the two touched files hold no colour literals; the raw palette classes removed here are the token gate's domain. Bundle +19.2 KiB gzipped (first consumer of Select), inside the 100 KB budget.
What
Unit U13 of the Studio design-system plan: the Renders panel sweep.
Select. No native<select>is left in the Renders tab.className. Both Exports now measure the same.IconButton, each wrapped in the sharedTooltipcarrying what thetitleattribute used to say.text-4,bg-input,border-border,danger,accent,text-step-*).SelectOptiongains an optionaldisabled, passed through to Base UI'sSelect.Item.Why
R8 asks for exactly one implementation of every control, and no native
<select>: a native popup is the operating system's, so its radius, surface and type belong to someone else and no token can reach them.AE3 asks that the header Export and the Renders Export share height, radius, fill and type size. They did not. The panel's Export carried its own type size in
className, andcnresolves that by dropping the size recipe's own step, so the two Exports sat a type step apart. The captured computed-style table, before and after:The
disabledflag onSelectOptionis not new behaviour, it is preserved behaviour. The Resolution list disables a preset that is not an exact integer upscale of the authored size, because the producer rejects that scale at render time. Without the flag the shared Select would have offered a choice that fails the render.How
Option lists become data (
FORMAT_OPTIONS,QUALITY_OPTIONS,FPS_OPTIONS, and the Resolution list built per render from the composition's dimensions), which is the shapeSelecttakes. EachonCommitkeeps the literal-union cast and the persistence side effect itsonChangehandler had. Frame rate crosses the string boundary explicitly:String(fps)in,Number(next)out.The quality option list also carried a
titlefield that nothing ever rendered; it is gone.Test plan
RenderQueue.test.tsx, all through the component's public render, driven with the pointer and key sequences a browser produces:w-full. Set equality, not "contains", so an extra class that displaces a recipe class fails. Proved non-vacuous by re-adding the type-size override: the assertion fails naming the dropped step.querySelector("select")returns null.isTypingTargetand by the playback-shortcut selector exactly as a native<select>is, and both are asserted true rather than merely equal."mov", the literal union member, not the"MOV (ProRes)"label. Read back through the real store, not a spy.landscape-4kpreset.disabledpass-through.Also run: the full Studio suite (440 files, 4841 tests, green, including the token gate and the hex ratchet),
typecheck, the Studio build,oxlint,oxfmt --check,fallow audit --base origin/main --fail-on-issues(exit 0), and the design-shots capture before and after against this branch's base.Bundle: the app's gzipped JS and CSS go from 1,573,539 to 1,593,184 bytes, +19,645 bytes (+19.2 KiB). This is the first consumer of
Selectin the shipped app, so it is where Base UI's select entry point lands. Nothing trimmed.Not covered
red-400,neutral-700,bg-red-500/10) are Tailwind palette classes, which the token gate governs, not the hex ratchet. The ratchet test fails on a fall as well as on a rise, and the suite is green, which is the evidence.<button>. It is an 80x45 media surface, not a control on the 24/28/32 grid, andIconButtonwould force it into a square.FormatInfoTooltipstays a local popover. Its content is a heading plus three descriptions, which the one-line sharedTooltipdoes not carry; the file already says so.FfmpegRequiredNotice.tsxis not in this unit's file list and is untouched. Its amber card still uses palette classes.Before
Origin main's current tip (f24ff83), on the repo's own
swiss-gridexample resized to 1280x720. Each image below is a combined before/after sheet (before on the left, after on the right).After
This branch (16b6c82). Only #3772's own diff shows here: the four selects on the shared Select, the disabled-option label staying reachable by keyboard, and the panel's icon buttons. Export's box is identical on both sides (already on the shared Button primitive via #3622; this PR drops a redundant class with no visible effect).
Measured computed values and the isolation note are in the capture README; this replaces an earlier capture pass I caught using a stale BEFORE commit.