Skip to content

fix(ui): improve selected and hover contrast across the app - #140

Open
cashcon57 wants to merge 8 commits into
AtomicBot-ai:mainfrom
cashcon57:fix/selected-state-contrast
Open

fix(ui): improve selected and hover contrast across the app#140
cashcon57 wants to merge 8 commits into
AtomicBot-ai:mainfrom
cashcon57:fix/selected-state-contrast

Conversation

@cashcon57

@cashcon57 cashcon57 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Describe Your Changes

A pass over the app's low-contrast selected/hover states. Several used an absolute
bg-secondary / bg-accent overlay, which collapses against light backgrounds
(both ≈0.97 in light, nearly identical to the surfaces they sit on) — so the
state was hard to see, especially in light mode, and in places hover read as strong
as (or stronger than) the selection.

Everything moves to a background-relative foreground overlay so it stays
legible on any surface in both themes, with hover (/10) a clear step below
selected (/20)
.

Design decisions

  • Relative overlay, not an absolute token. bg-secondary/bg-accent are ≈0.97
    in light — the same lightness as the surfaces behind them, so they vanish. A
    foreground overlay is a fixed delta from whatever is behind it, so it holds up
    on any surface and flips correctly between themes.
  • /10 hover, /20 selected. Two fixed steps keep hover clearly below selected;
    before, several menus had them equal (or hover stronger).
  • Fixed at the source for shared components. The ghost button variant and the
    base dropdown-menu item states are each changed once, so every ghost button (~80)
    and every dropdown inherits it rather than being patched per call site.
  • outline variant left alone — it has a border, so its hover already reads fine.
  • Styling only — no logic, state, or dependency changes.

Selected states → bg-foreground/20

  • Model selector (DropdownModelProvider), search dialog (SearchDialog), Hermes
    Agent / Claude Code model rows, dropdown control (DropdownControl), assistant
    switcher (SamplerPopover). The search dialog's selected row was previously
    lighter than its hover — now clearly stronger.
  • Hub sort dropdown (hub/index) and default-assistant picker (settings/assistant)
    — the selected item used bg-secondary (0.97), the exact same lightness as the
    menu hover, so selected and hovered items were indistinguishable.

Hover states → bg-foreground/10

  • ghost button variant (components/ui/button.tsx) — hover:bg-accent was the
    faint hover on every ghost button (message copy/edit/delete/regenerate, header
    icons, menus, …). Shared design-system change — affects all variant="ghost"
    buttons app-wide (~80 usages).
    outline is left as-is (it has a border).
  • Menu item highlight (components/ui/dropdown-menu.tsx) — the base
    focus:bg-accent / data-[state=open]:bg-accent states. Also a shared
    design-system change — affects every dropdown menu app-wide
    , and makes menu
    items hover consistently with ghost buttons.
  • Settings menu (SettingsMenu) — category links used hover:bg-secondary while
    provider rows used hover:bg-secondary/60 (mismatched and faint in light); unified.
  • Model edit / delete trigger icons (EditModel, DeleteModel) — these had no
    hover feedback at all
    ; added.
  • Download panel pause / resume / cancel controls (DownloadManegement) — these were
    variant="secondary" on a bg-secondary row, so the button was the same shade as
    its surface and hover:bg-secondary/80 only faded it further. Switched to ghost
    so they share the hover overlay and read on the row.
  • A final sweep of the remaining faint row/icon hovers (hover:bg-{muted,secondary}
    at assorted opacities): hub model variant rows (HubModelCard), project file rows
    (ProjectFiles), security config rows (SecurityConfigDialog), assistant cards,
    and the small utility icon buttons in AddEditMCPServer, AddEditCustomCliDialog,
    AudioPlayer, Capabilities, ApiKeyInput, AttachmentChip, hermes-agent.
    These had drifted across several ad-hoc values; they now share the one hover overlay.

Companion to #106 (sidebar/nav/settings selected states, already merged).
Styling-only — no logic, no new dependencies — verified in a local dev build.

Before / After

Light mode (where the change is most visible) — stock v1.1.119 vs. this branch:

atomic-contrast-01b

Model selector — row hover

atomic-contrast-02

Hub sort dropdown — selected vs. hover (were the same shade before)

atomic-contrast-04

Model edit button — hover (no hover before)

atomic-contrast-05

Search (⌘K) — highlighted result

atomic-contrast-03

Settings menu — hover vs. selected

Fixes Issues

Self Checklist

  • Added relevant comments, esp in complex areas — n/a (CSS-class-only change)
  • Updated docs (for bug fixes / features) — n/a
  • Created issues for follow-up changes or refactoring needed — n/a

@cashcon57 cashcon57 changed the title fix(ui): improve contrast of selected items in dropdowns and dialogs fix(ui): improve selected and hover contrast across the app Jun 30, 2026
@cashcon57
cashcon57 force-pushed the fix/selected-state-contrast branch from ff3c603 to 9509040 Compare June 30, 2026 19:23
@cashcon57
cashcon57 marked this pull request as ready for review July 7, 2026 18:45
@cashcon57
cashcon57 force-pushed the fix/selected-state-contrast branch from 8e3ae69 to f3777d8 Compare July 31, 2026 17:10
@cashcon57
cashcon57 requested a review from Vect0rM as a code owner July 31, 2026 17:10
@cashcon57

Copy link
Copy Markdown
Contributor Author

Rebased onto current main to keep this easy to merge and resolve conflicts. Typecheck passing.

Vect0rM commented Aug 20, 2026

Copy link
Copy Markdown
Member

Thanks for this, @cashcon57, and apologies for how long it's been sitting. The direction is right, and I want to say why before getting to the changes, because the core insight is good.

Replacing bg-accent and bg-secondary/40 with bg-foreground/10 for hover and bg-foreground/20 for selected is the correct call, for two reasons you may not have spelled out: it matches the treatment SettingsMenu was already using for its active state ([&.active]:bg-foreground/20), so the app converges on one system rather than gaining another; and because foreground already inverts per theme, the dark: special-cases you deleted — dark:hover:bg-accent/50, hover:dark:bg-secondary/60 — genuinely become redundant rather than being dropped on the floor. A 10%/20% split between hover and selected also keeps the two states distinguishable, which was the actual complaint.

Three things before I can merge.

1. The screenshots are in the commit

atomic-chat-sidebar-before.png   425 KB
atomic-chat-sidebar-after.png    416 KB

Both at the repository root. That's 840 KB in git history forever, for two files nothing references — every future clone pays for them. Please drop them from the branch and attach them to the PR description instead, where they're much more useful anyway. I'd genuinely like to see them; a before/after is the only real way to review a contrast change.

2. It no longer merges

Two conflicts against current main:

  • web-app/src/containers/HubModelCard.tsx — deleted on main, modified on your branch. The modify/delete resolution is to drop your change to that file.
  • web-app/src/routes/hub/index.tsx — content conflict.

3. The sweep is now partial

This is the one worth a conversation. main has moved a lot in seven weeks, and checking your branch I count 41 remaining sites using the treatments this PR replaces — including plain hover:bg-accent in code added since you opened it:

src/containers/AgentSkillSlashMenu.tsx:53      hover:bg-accent/60
src/containers/AgentTaskSuggestions.tsx:49     hover:bg-accent
src/containers/AgentWorkspaceLayout.tsx:222    hover:bg-accent
src/containers/AgentWorkspacePreview.tsx:191   hover:bg-accent
src/routes/skills/index.tsx:159                hover:bg-accent

Merged as-is, the app ends up with both treatments side by side — the sidebar and dropdowns on the new one, the agent surfaces on the old. That's arguably worse than the inconsistency you set out to fix, because it's inconsistency between adjacent panels rather than between the app and itself.

Two ways forward, your call:

  • Finish the sweep — extend to the agent containers and the skills route so the whole app lands on one system.
  • Scope it explicitly — narrow to the shared primitives (button.tsx, dropdown-menu.tsx) plus SettingsMenu, say so in the description, and I'll open a follow-up issue for the rest.

I'd take either. The outline and secondary button variants and the destructive dropdown states should keep their own treatments regardless — those aren't part of this.

One thing I couldn't check: I can't render the app from here, so I'm taking the contrast improvement on your testing rather than measuring it. If you have before/after numbers against WCAG for the selected state, put them in the description — that would settle it for good.

Sorry again for the delay. Rebase, drop the PNGs, and tell me which scope you want, and I'll get this in 🎨


Generated by Claude Code

They were variant="secondary" on a bg-secondary row, so the button
was the same shade as its surface and the hover (secondary/80) faded
it further. Switch to the ghost variant so they pick up the shared
hover:bg-foreground/10 overlay and read on the row.
…and dialogs

Extend the foreground/10 hover and foreground/20 selected treatment to the
sites added since this branch opened (agent workspace controls, skill slash
menu, task suggestions, skills route, hub model rows and filters, download
options, thread list, dialog close, drawer handle, model combobox, MCP
server toggle, cloud provider picker) so the app lands on one system.

Static container fills, status badges, the outline and secondary button
variants, and destructive dropdown states keep their own treatments.
@cashcon57
cashcon57 force-pushed the fix/selected-state-contrast branch from f3777d8 to bc8f17a Compare August 20, 2026 18:10

Vect0rM commented Aug 21, 2026

Copy link
Copy Markdown
Member

Thanks for taking the wider option, @cashcon57 — all three items from last round are closed, and I appreciate you choosing to finish the sweep rather than narrow the scope.

Verified on your branch (based on 3b625ff, fast-forwards — no conflicts left):

  • PNGs are gone from the branch, and the before/afters are in the description where they're actually useful. The light-mode comparison makes the case on its own.
  • tsc -b — exit 0. eslint on all 38 changed files — 0 errors (2 react-refresh warnings, both present on main).
  • prettier --check flags 15 of your files — and flags the same 15 on main. Pre-existing drift, not yours. Please don't reformat them here; it would bury the diff.
  • The sweep is genuinely complete. main has 51 sites on the old treatments; your branch has 5, and all 5 are the exclusions we agreed: the outline and secondary button variants, plus three static bg-secondary/40 container fills (AudioPlayer:190, settings/assistant:126, launch/index:1143) that were never hover states. Destructive dropdown states are intact.

Two things before merge. The first is a genuine regression, and it's my fault for not warning you about it.

1. Two tests fail

Full vitest run: main is 220 files / 2143 tests, all green. Your branch is 2 files / 2 tests red. Both assert the old tokens:

src/containers/__tests__/ThreadList.test.tsx:187
  › highlights the open thread card inside a project
  AssertionError: expected [ 'bg-card', …(9) ] to include 'bg-secondary'

src/containers/__tests__/ModelCombobox.test.tsx:496
  expect(modelElement).toHaveClass('bg-secondary')

These landed on main after you opened this, and they pin the exact tokens you're replacing — ThreadList's even has a comment explaining it matches the exact token so dark:bg-secondary/20 can't give a false positive. Both should move to bg-foreground/20, keeping the exact-token matching.

2. Hover overrides selected on ~12 sites

This is the interesting one, and it's the same class of bug the PR exists to fix.

I built the branch and read the compiled CSS rather than guessing at precedence:

.bg-foreground\/20{background-color:color-mix(in oklab,var(--foreground)20%,transparent)}
.hover\:bg-foreground\/10:hover{background-color:color-mix(in oklab,var(--foreground)10%,transparent)}

(0,1,0) versus (0,2,0). On any element carrying both, hovering a selected row drops it to the lighter hover shade — selection appears to dim under the cursor.

You clearly know this, because you guard it in nine places with bg-foreground/20 hover:bg-foreground/20SearchDialog ×4, DropdownModelProvider ×2, DropdownControl, claude-code, hermes-agent. The guard is missing at:

ThreadList.tsx:188
hub/ModelListRow.tsx:44
hub/DownloadOptionsSelect.tsx:228
routes/skills/index.tsx:160
SettingsMenu.tsx:211, :270, :324

Plus the DropdownMenuItem sites, which inherit focus:bg-foreground/10 from the primitive you changed — Radix focuses menu items on pointer move, so it's the same collision: HubFilters.tsx:122, settings/assistant.tsx:109, SamplerPopover.tsx:158 and :169, AddEditMCPServer.tsx:391.

ModelCombobox.tsx:160-161 is the same bug by a different route:

value === model && 'bg-foreground/20 shadow-sm',
highlightedIndex === index && ' bg-foreground/10'

Both are unprefixed utilities, so cn()'s tailwind-merge collapses them and the last one wins — I can see it in the failing test's DOM dump, where bg-background was dropped in favour of bg-foreground/10 on the highlighted row. Since onMouseEnter sets highlightedIndex, hovering the selected model renders it at hover strength. Deterministic, not a specificity tie.

AgentSkillSlashMenu.tsx:52 is the pattern to copy — a ternary, so the two states can never both apply.

Worth closing all of them: "hover reads as strong as the selection" is the exact complaint in your own description.

Nits

  • SearchDialog.tsx:349 picked up a stray {' '} after the .map() close, inside <div className="p-1">. Unrelated to the sweep — an editor artifact, I'd guess. Drop it.
  • The description still lists HubModelCard (deleted on main) and predates the extended sweep, so it undersells what actually landed. Worth a refresh, since it's what ends up in the merge commit.
  • Still happy to see WCAG before/after numbers for the selected state if you have them — not a blocker, the screenshots carry it.

Fix the two tests and the selected/hover collision and I'll merge this. Seven weeks is far too long for a PR this well-argued to have sat 🎨


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: low-contrast selected/highlighted states (most visible in light mode)

2 participants