feat: SW-2563 unified selected state for Toggle & ToggleGroup - #196
Conversation
Replace the solid-primary selected fill with one consistent state language: a soft tint (--selected) + primary text + pressed inset, per-item borders so a segmented group is always outlined (fixes the default variant having none) with dividers that keep it countable when all are selected, and a content-aware dotted-ring→check indicator on labelled items (label-only on by default; icon-only/icon+label off; ToggleGroupItem selectedIndicator overrides). - tokens: --selected / --selected-foreground / --selected-border / --shadow-inset-pressed (derived from --primary/--foreground), exposed as bg-selected / text-selected-foreground / border-selected-border. - Toggle on-state now tint + pressed (was bg-accent). - ToggleGroup: drop the SW-2292 solid override; per-item borders + collapse for all variants so the border and rounded corners share one element and the selected fill always aligns. - stories + play tests for the indicator and the variations. Button/ButtonGroup selection is handled separately (SW-2445). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…W-2563 Revert the Toggle/ToggleGroup changes and the shared --selected tokens to main — those now live in SW-2563 (#196). This PR keeps only the Button aria-pressed selected state (scoped to outline/ghost) and the ButtonGroup variations story. Depends on #196 landing first for the --selected tokens. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Include the --selected / --selected-foreground / --selected-border / --shadow-inset-pressed tokens so the button aria-pressed selected state renders without depending on SW-2563 (#196) merging first. These tokens are identical to #196's; whichever PR merges second will have a trivial index.tailwind.css conflict (keep one copy). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The aria-pressed treatment on Button referenced bg-selected / text-selected-foreground / --shadow-inset-pressed, but those tokens only existed in #196. Tailwind silently emits no CSS for a utility whose theme key is missing, so merging this before #196 would have shipped a selected state that renders nothing, with CI green. Moves the four token lines from #196 into this PR (verbatim, so #196 stays in sync) and adds assertions that fail if they ever go missing. Also covers the vertical + separator case: the horizontal/vertical variants strip the preceding child's border when a separator follows, so the separator is the only thing drawing that divider. The story now exercises it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The segmented border was applied as group-data-[spacing=0]/toggle-group:border, which compiles to specificity (0,2,0). A consumer passing border-r-0 through className lands at (0,1,0) and silently loses, so an item can no longer sit flush against an adjacent control. PlateMapPlateSelector relies on exactly that: each plate tab passes "rounded-r-none border-r-0" so it butts against its remove button. On main the border came from toggleVariants as plain .border, tying at (0,1,0) and losing to the later .border-r-0 — the override worked. This restores that. Derives the segmented/orientation classes in JS and emits plain utilities, so className keeps winning. Adds a Variations row covering the joined-control composition with an assertion on the computed border width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
|
Reviewed this and pushed one fix ( 1. Fixed: buttons couldn't sit flush against each other any more 🔴In plain terms: this PR gave every segmented toggle item its own outline — good, that's the point. But it applied that outline in a way that outranks any instruction a screen sends in. So when a screen said "remove the right edge of this one so it joins seamlessly onto the button beside it", that instruction got ignored. Where it showed: the plate tabs in PlateMapEditor. Each tab has a little ✕ delete button welded onto its right side. The tab is supposed to drop its right edge so the two read as one control. With this change, the edge came back — so you'd get a doubled, slightly darker seam between the tab and its ✕. Think of it as a specificity argument: the new rule was shouting louder than the screen's own instruction, so the screen lost. The fix: same visual result, same outline on every segmented item — just applied at normal volume, so a screen's own styling wins again, the way it does everywhere else in the kit. Nothing about the intended look changes. I also added a row to the Variations story showing this joined pattern (a tab welded to a ✕ button), with a test that fails if the edge ever comes back. It's worth seeing in the story anyway — it's a real composition people use. 2. Worth knowing: the ring appears based on a guess about your content 🟡The dotted-ring indicator turns itself on for "label-only" items and off for items with icons. Sensible default. But the check for "does this have an icon" is really "does this contain anything other than bare text". What that means in practice: if a label is wrapped in anything — a The risk isn't one item losing its ring; it's a group where some items have plain text and others have wrapped text. Then some items reserve space for the ring and some don't, so the labels don't line up. Slightly ragged, and hard to trace back to the cause.
Not blocking, and I haven't changed it. Two ways to firm it up if you want: detect actual icon elements rather than "any element", or document that wrapping a label opts out of the ring. Flagging it because it'll be confusing the first time someone hits it. 3. Nobody has seen this running 🟡The Vercel preview for this PR is marked Ignored, so no live Storybook was ever built for it. That means:
The 9 green checks are real, but they're lint, typecheck, build, and the component tests — not a visual pass. Someone should get a preview deployed and actually look at this before merging, especially the plate tabs. For what it's worth, the component tests in here are good — they check real rendered borders and count items, rather than just asserting that props got passed through. 4. Heads-up: this reverses an earlier decision 📌The previous look — selected items filled solid primary blue — was a deliberate call (SW-2292). This replaces it with the soft tint. The reasoning in the description is sound and I agree with it, but whoever made that original call should get a nod rather than finding out at merge. Consistent with #195 ✅Two things I checked line up with the Button work:
One merge choreThe four So these two PRs will conflict on those lines. Drop them from this PR and keep #195's. The values are identical, so nothing here changes. Merge order no longer matters either way. |
![unblocked[bot]](https://avatars.githubusercontent.com/in/166219?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No issues found
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
…ating The tokens now live in #195, which needs them to render anything at all. Keeping a second copy here means whichever PR merges second hits a conflict on identical lines, so drop ours and consume what #195 lands on main. This makes the PR depend on #195 merging first. Adds a play-test assertion on the resolved selected background so that dependency fails loudly: without the tokens Tailwind emits no css for bg-selected and does not warn, so the selected state would otherwise render as nothing with CI green. Expect this branch to be red until #195 is on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
|
Two updates — and this PR's checks will now go red on purpose. Please read before reacting to the failure. Removed the duplicated
|
Status check — what's neededNo unresolved review threads.
What to do: merge latest Coordinate before merging:
|
Correction — the Storybook E2E failure is real, not staleI tried refreshing with Root cause: the but commit So this PR can't go green on its own. Two ways forward:
Either way it's a merge-order decision between #195 and #196, not a CI refresh. I did not push anything — the branch is untouched. |
…dren The dotted-ring default was derived by inspecting React children: any element child counted as "has icon", so a label wrapped in a <span>, a translation component or a tooltip silently lost its ring. A group mixing plain and wrapped labels would then reserve the indicator slot on some items and not others, misaligning the labels. The indicator is now always rendered when the prop is unset and hides itself via `group-has-[svg:not(<indicator> svg)]/toggle:hidden` — i.e. the decision is made from the rendered DOM (does the item paint another svg?) rather than from the children's shape. `display:none` drops the slot too, so there is still no layout shift. `selectedIndicator="dot" | "none"` keep overriding as before. PlateMapPlateSelector opts out explicitly (`selectedIndicator="none"`): its tabs previously had no ring only because their label happens to be span-wrapped. The Variations story gains a row + assertions covering wrapped label (ring kept), icon + label (ring hidden) and a forced "dot". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
54321jenn-ts
left a comment
54321jenn-ts
left a comment
There was a problem hiding this comment.
LGTM but merge after #195
3 participants
SW-2563 — A unified “selected” state for Toggle & ToggleGroup
The usability problem
In a
ToggleGroup, users couldn't reliably tell what was selected — or that items were selectable:bg-primaryfill, read only by contrast with unselected neighbours; select every option and the segments merged into one seamless primary pill.Before → after
Before — solid fill; labelled default-variant has no outline; all-selected merges:
After — soft tint + primary text + a dotted-ring→check indicator on labelled items, per-item borders (always outlined) with dividers, so every selection is legible and countable:
What changed (Toggle / ToggleGroup only)
src/index.tailwind.css):--selected,--selected-foreground,--selected-border,--shadow-inset-pressed— derived from--primary/--foregroundso they track light/dark with no separate.darkblock — exposed asbg-selected/text-selected-foreground/border-selected-border.Toggle: on-state is the tint + pressed inset (wasbg-accent);outlinegains a selected border.ToggleGroup: dropped the SW-2292 solid-primary override (the base tint now applies). Every segmented item is bordered with edge-collapse (not onlyvariant="outline"), so the group is always outlined, stays countable when all are selected, and — because the border and rounded corners live on the same element — the selected fill always aligns at every size. NewToggleGroupItempropselectedIndicator("dot" | "none") renders a faint dotted ring that cross-fades to a check, in a fixed-width slot (no layout shift). It defaults by content: label-only items get the ring; icon-only and icon+label items get none (the icon carries it, no double icons) — the prop overrides.Tests
Toggle/ToggleGroupplay tests pass.SW-T5647,SW-T5649.yarn typecheck,yarn lint,yarn buildclean (dist ships the var-basedbg-selectedutilities).Scope
Toggle/ToggleGroup only —
Button/ButtonGroupselection (aria-pressed) is handled separately (SW-2445). The shared--selectedtokens live here.🤖 Generated with Claude Code