feat: SW-2445 Button/ButtonGroup selected state (aria-pressed) - #195
Conversation
Replace the solid-primary selected fill with one consistent state language — a soft tint (--selected), primary text, a pressed inset, segment dividers, and an always-outlined segmented container — so selection reads without relying on an unselected neighbour and 'all selected' no longer collapses into one button. - tokens: --selected / --selected-foreground / --selected-border / --shadow-inset-pressed (derived from --primary/--foreground; theme-tracking), exposed as bg-selected / text-selected-foreground / border-selected-border. - Toggle: on-state now tint + pressed (was bg-accent); outline gets a selected border. - ToggleGroup: drop the SW-2292 solid override (base now tints); segmented groups are always outlined with dividers between items (fixes default-variant having no border); new ToggleGroupItem selectedIndicator="dot" renders a dotted-ring resting affordance that cross-fades to a check (fixed-width slot, no layout shift on click). - Button: aria-pressed now styled with the same selected language, so a segmented button group can show its active item (previously aria-pressed had no visual state). - story + play test for the selected indicator; new story testCaseId left "". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
GitHub Actions
Inactive
![unblocked[bot]](https://avatars.githubusercontent.com/in/166219?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue 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.
…ator Default the indicator by item content instead of a fixed "none": label-only items (text, no icon) show the dotted-ring→check by default; icon-only and icon+label items default to "none" so there aren't two icons side by side. The explicit selectedIndicator prop still overrides either way, so a team can opt into the ring on icon items if desired. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
Move the aria-pressed tint off the base class (where it clobbered filled variants) onto the outline and ghost variants only. Fixes the AssistantDockControls regression where the active secondary dock button lost its bg-secondary fill to the faint --selected tint; primary/secondary/destructive buttons now keep their own background when aria-pressed. Addresses PR review on button.tsx. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
ButtonGroup: command / single-active (aria-pressed) / toggle-buttons / text+separator / vertical. ToggleGroup: single icon-only vs label-only (indicator default), multi all-selected, outline+spaced, and sizes. New-story testCaseIds left "" for the zephyr_sync workflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
…up collapse) ButtonGroup collapses each non-first button's left border, so the outline variant's aria-pressed:border-selected-border only recoloured 3 sides of an active segment — the selection outline looked asymmetric / missing a left border. Rely on the tint fill + pressed inset instead (matching ToggleGroupItem, which goes border-0 in a segmented group); segment borders stay uniform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
Replace the container border (added earlier) with per-item borders + edge collapse for every variant (not just outline), matching ButtonGroup. The container border nested a rounded item fill inside a rounded border, so the selected fill's corners overflowed/misaligned (worst at size=sm, where the container radius differed from the item radius). With the border and the rounded corners on the same element, the selected fill always aligns; the default variant still gets the outline, and adjacent borders collapse into a single divider. Updated the play test to assert per-item borders. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
…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>
GitHub Actions
Inactive
boramyi-ts
changed the title
GitHub Actions
Inactive
0d92cec to
3526fad
Compare
GitHub Actions
Inactive
Drop the my-px/mx-px inset so the separator is flush with the buttons instead of 1px shorter top/bottom, which read as a gap next to an adjacent segment (e.g. between a button and a ButtonGroupText). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
Three related fixes to how a ButtonGroup renders separators and text labels: - Drop the trailing border on the element immediately before a ButtonGroupSeparator, so the separator is the single divider instead of button-border + separator rendering as a doubled line. - Separator color bg-input -> bg-border, matching the segment borders (bg-input was deliberately darker for form fields and stood out). - ButtonGroupText: remove the grey bg-muted fill; render as quiet muted-foreground / font-normal inline text divided by the separator, rather than a filled block welded onto the buttons. Matches the look MessageBranchPage already hand-rolls for the same "X of Y" pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
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>
GitHub Actions
Inactive
|
Reviewed this and pushed two fixes to the branch ( 1. The selected state would have done nothing at all 🔴The problem, in plain terms: this PR tells buttons to use a colour called The nasty part: nothing would have told you. Tailwind (our styling tool) doesn't warn about a missing colour — it silently drops the rule. All 10 CI checks would still pass. The Storybook preview would look normal-ish. You'd only find out when someone opened a segmented button group in the real app and asked why nothing highlights. I confirmed this rather than guessing — compiled the branch's styles and checked whether the rule came out the other end: What I did: moved the four colour definitions out of #196 and into this PR, copied character-for-character so nothing drifts between the two. This PR now stands on its own — no more "must merge #196 first". One thing you need to do: #196 still has its own copy of those same four lines, so the two PRs will now collide in 2. Vertical button groups could lose their divider line 🟡The problem: this PR added a smart rule — when a divider sits between two buttons, remove the button's own border so you don't get a doubled-up 2px line. Good change. But it assumes the divider is actually drawing a line. In a vertical stack, the divider needs to be told it's horizontal ( Nobody would have caught it, because the new Variations story shows a vertical group and a group with a divider, but never both together. What I did: added a "Vertical with separator" row to the story that uses both at once, plus a test that fails if the divider ever has zero height. 3. The tests weren't really testing the feature 🟡The original test checked that a button you'd written What I did: added a test that reads the button's actual rendered colour and checks two things — that it resolves to a real paint (not transparent), and that it differs from an unselected button next to it. That test fails if the colours ever go missing again. 4. Something in this PR changes screens outside its scope 🟡 — needs your eyesThis is the one I can't decide for you. Alongside the Concretely, the text label loses its grey filled-in background and gets lighter, quieter text — it now reads as a caption rather than a button-shaped chip. Where that shows up today: the zoom percentage readout in PlateMapEditor (the The other place it's used ( I've documented all of this in the PR description rather than reverting it — the change looks deliberate and arguably better. But please open the preview and look at the zoom control before merging, and loop in design if that pill was intentional: 👉 https://ts-lib-ui-kit-storybook-git-sw-2445-select-fd55d1-tetra-science.vercel.app Verified as fine ✅
One judgement call for design 🎨Hovering a selected button now does nothing visually — the selected tint wins over the hover colour. So the active segment gives no hover feedback. That's a defensible choice ("selected sticks"), and I've left it as-is, but it's worth a nod from design since it wasn't an explicit decision. What I couldn't checkI could not run the interactive Storybook tests locally — this checkout's TL;DRFix 1 was a real "ships broken, tests pass anyway" bug — that one mattered. Fixes 2 and 3 are safety nets. Item 4 just needs you to look at the preview and confirm the zoom readout is meant to change. Then it's good to go. |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
|
Design review done on the preview — both open visual questions are now closed. Nothing further needed from the author on these. 1. 2. Pressed-in look on selected buttons — reviewed and accepted. The Consistent with that, the hover behaviour also stays: hovering a selected button shows no hover colour, because the selected tint outranks it. Selected sticks. CI is fully green, including Storybook E2E. The new assertions ran for real in Chromium: That the colour assertion passes is the meaningful bit — it confirms the Only remaining item is the #196 merge conflict: both PRs now define the same four token lines in |
…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>
boramyi-ts
changed the title
boramyi-ts
changed the title
|
Thanks @54321jenn-ts — verified all four items on the branch ( Fixed the one red check: the PR Title Checker was failing because the title didn't start with the Jira number — retitled to Two follow-ups, neither on this PR's code:
|
3 participants
SW-2445 — Button / ButtonGroup selected state (
aria-pressed)What & why
aria-pressedon aButtonhad no visual state, so a segmented button group couldn't show which option was active. This adds a selected state so toggle-style button groups read correctly.Button:aria-pressednow gets the shared selected treatment — soft--selectedtint + primary text + pressed inset — scoped to theoutlineandghostvariants (the ones used for toggle/segmented groups). Filled variants (default/secondary/destructive) keep their own background, so an activesecondarybutton (e.g.AssistantDockControls) is unaffected.ButtonGroupcollapses each non-first button's left border, so a colored selected border would only cover 3 sides and look asymmetric. The tint + pressed inset carries the state; segment borders stay uniform.ButtonGroup"Variations" story (SW-T5648): command group (no selection), single-active (aria-pressed), toggle-buttons (multiplearia-pressed), text + separator, vertical, and vertical + separator.Token ownership (changed from the original plan)
The
--selected/--selected-foreground/--selected-border/--shadow-inset-pressedtokens are defined in this PR'ssrc/index.tailwind.css.They were originally kept in #196, but Tailwind v4 emits no CSS at all for a utility whose theme key is missing — it does not warn or fail. Merging this PR first would therefore have shipped an
aria-pressedstate that renders nothing, with all CI checks green. Defining them here removes the merge-order dependency entirely.#196 follow-up: #196 still adds the same four token lines, so the two PRs will conflict in
src/index.tailwind.css. Resolve by dropping them from #196 and keeping this PR's copy — the values are byte-identical, so nothing else in #196 changes.Also in this PR:
ButtonGroupText/ButtonGroupSeparatorrestyleNot strictly part of the
aria-pressedwork, but included because the separator change is what makes selected segments line up. Calling it out because it changes existing screens:ButtonGroupText: dropsbg-muted,font-medium→font-normal, addstext-muted-foreground. The text chip now reads as a label rather than a filled segment.ButtonGroupSeparator:bg-input→bg-border, drops themx-px/my-pxinset so the divider sits flush. Paired with a new rule that strips the preceding child's border, so you get one clean 1px divider instead of a doubled line.Known visual impact:
PlateZoomControl— the zoom percentage readout loses its muted chip fill.MessageBranchPagealready overrides these classes and is unaffected. Both worth an eyeball on the preview before merge.Selection semantics
ButtonGroupstays presentational (no built-in selection) — you mark the active childaria-pressed. For true single/multi-select semantics,ToggleGroup(#196) remains the purpose-built option.Tests
ButtonandButtonGroupplay tests; typecheck + lint clean. The Variations story now asserts the selected tint actually resolves to a paint and differs from an unpressed sibling — so if the tokens ever go missing again, the test fails instead of silently rendering nothing.🤖 Generated with Claude Code