fix(ui): update dropdown font + bg color#372
Conversation
ConnysCode
left a comment
ConnysCode
left a comment
There was a problem hiding this comment.
Review: Approve — clean, well-reasoned fix. The CSS logic is sound, the hex values match the tokens they claim to mirror, and the test asserts the right thing. No blockers; two minor, non-blocking notes below for an optional follow-up.
Grounded against the real head (1fa994e):
- The three-way
[data-theme]partition is correct.data-themeon<html>is only everlight,dark, or absent — the pre-hydration bootstrap inapp/layout.tsx:54writes the palette todata-palette(lagoon/petrol/atelier) and only ever setsdata-themetolight/dark. So[data-theme='dark']→ dark hex,:not([data-theme='dark'])→ light hex (coverslight+ system-light), and theprefers-color-scheme: dark+:not([data-theme])media rule correctly overrides system-dark (same specificity, later in source → wins). All four appearance states resolve to a readable contrast pair. ✅ - The hex values do mirror the tokens.
--bg-surface-raised-top: light-dark(#FFFFFF, #303440)and--text-primary: light-dark(#1B1D24, #EEEFF3)in_lib/theme.css:49,58— exact match. ✅ - The test values are accurate.
PALETTES = ['lagoon','petrol','atelier']+THEMES = ['system','light','dark'](ThemeControls.tsx:22,25) → 2 selects, 6 options, in that order.renderWithIntlwraps inNextIntlClientProvider, souseTranslations()resolves. ✅
🟡 Scope — the fix is app-wide, not just the three named dropdowns
This is a global select option rule, so it now governs every <select> in the app (the admin pages, AgentPicker, SelfExtensionPanel, … — ~10 selects), not only the palette/appearance/locale ones from #360. That's the right outcome — readable Windows option popups everywhere — and I verified none of the other selects carried their own <option> color styling, so nothing gets silently overridden. Worth stating in the PR description so the broadened blast radius is intentional and documented rather than incidental.
PR description — factcheck
| Claim | Reality |
|---|---|
| "Hex values mirror --bg-surface-raised-top / --text-primary" | ✅ Exact match (theme.css:49,58). |
| "CSS + JSX class removal" | ✅ Only globals.css additions + className removal on <option>s + one new test. |
| "Risk: Low" | Agreed — with the note that it's now app-wide (see Scope above). No conflicting option styles found elsewhere. |
| "npm run lint && typecheck && test" | Not re-run by this review (web-ui deps not installed). The added test is statically correct; logic verified by inspection. |
Minimum to merge
Nothing required. The two notes below are optional polish.
Minor / nits (non-blocking)
- Test covers only
ThemeControls, notLocaleSwitcher. This PR removed the per-<option>classNamefromLocaleSwitcher.tsxtoo, but onlyThemeControlshas a regression guard. Consider extending coverage so the class can't silently creep back on the locale select. (inline) - Hex duplicates the tokens — acknowledged in the PR's
keep in synccomment, and genuinely unavoidable here: the Windows combobox widget ignoresvar(--…)on<option>, so the literal hex has to live in the rule. No action; flagging only that thekeep in synccomment is the right mitigation. (inline)
(Reviewed against head 1fa994e.)
| up honoring the background-color or only the color. Concrete hex values | ||
| mirror --bg-surface-raised-top / --text-primary in theme.css; keep in | ||
| sync if those tokens change. */ | ||
| :root[data-theme='dark'] select option { |
There was a problem hiding this comment.
This is a global select option rule — it now styles every <select> in the app, not just the three from #360. That includes the admin selects (admin/settings, admin/providers, admin/danger-zone, admin/users, admin/builder, admin/kg-priorities), AgentPicker, and SelfExtensionPanel. I checked each — none carry their own <option> color styling, so nothing gets overridden and this is a net improvement (readable Windows option popups everywhere). Non-blocking: please call out the app-wide scope in the PR description so the broadened blast radius is intentional rather than incidental.
The three-way partition itself is correct: data-theme is only ever light/dark/absent (the bootstrap in layout.tsx:54 routes the palette to data-palette), so dark → dark hex, :not([data-theme='dark']) → light hex, and the prefers-color-scheme: dark + :not([data-theme]) rule wins for system-dark by source order. ✅
| * className must not come back, or contributors would assume it does | ||
| * something. | ||
| */ | ||
| describe('<ThemeControls />', () => { |
There was a problem hiding this comment.
Good regression guard. Minor: this PR also removed the per-<option> className from LocaleSwitcher.tsx, but only ThemeControls is covered here — the comment's intent ("the per-option className must not come back") applies equally to the locale select. Consider adding a parallel assertion for LocaleSwitcher (or a presence check on the globals.css rule) so the class can't silently reappear there. Non-blocking.
2 participants
What
Fix unreadable native select dropdown options on Windows (#360).
Why
#360 Windows combobox widget ignores CSS vars on and doesn't inherit color-scheme into the popup — light-dark() resolved against dark page, painted near-white text on OS-light popup background. Moved option colors to [data-theme]-scoped global rules with hex values.
Test plan
Risk
Low. CSS + JSX class removal. Hex values mirror --bg-surface-raised-top / --text-primary; keep in sync.