ToggleGroupControl: add visual emphasis to selected item#75138
Merged
Conversation
|
Size Change: -10 B (0%) Total Size: 3 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
feat/toggle-group-control-polish
branch
from
0c52227 to
e10d05c
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ciampo
changed the title
Member
|
Is there any discussion or context to share with this? Noting since the design was the subject of much discussion in #74036. Though personally, to my untrained eye this feels like an improvement, as the "Before" as introduced in #74036 was quite subtle and perhaps an overcorrection from the earlier design. |
Contributor
Author
It was discussed with folks from @WordPress/gutenberg-design , although I don't believe there is a dedicated issue. |
Contributor
|
The broad context is really in this issue and specifically this comment. And it is a rabbit hole: the borders are there to ensure contrast in icon only modes, and to match adjacent controls like inputs and so on. The broad agreement was that the old variant, with the black total inversion for active state, drew undue attention. As far as this PR, it looks good to me. I think Gray 100 may be slightly too much, but arguably the followup here is to refine those variables through the token engine. @jameskoster WDYT? |
Contributor
|
Personally, I support this change for now. I suggested a similar change in this comment. |
Contributor
Author
I went with gray-100 to avoid using a custom value, and stick to existing variables instead. In terms of DS tokens, with Jay we observed that we'd likely use the As always, as we work on the |
Contributor
Author
|
Also, if we're ok with merging this PR, I'd ask for someone to approve it |
Contributor
|
eaeaea would be too dark for this. I don't think that's a blocker, but it needs to be arguably lighter, a la fcfcfc. I recognize that needs to fit into the system so let's connect on details. |
jasmussen
approved these changes
Feb 3, 2026
Contributor
Author
Absolutely 💯 |
adamsilverstein
pushed a commit
that referenced
this pull request
Feb 5, 2026
* ToggleGroupControl: use darker bg color for selected item * Update snapshots * CHANGELOG
Contributor
Author
|
Hey @jasmussen , I just spent some time pairing with @jameskoster on tweaking DS tokens (mostly in the context of #75204). We also looked at the token that we'd use for the background of the active item of Unfortunately, the current constraints of the DS don't allow for a much lighter color than
We feel that the best compromise would be to keep the value associated with the |
Contributor
|
Do you have a visual to share? I think it's important we get this right. It's 95% there, but given the ingredients we have, I feel like there are some iterations we can try in the beta period for this. |
Contributor
|
The token for the background of interactive elements in their hover/active state needs to be visible against all three application surface tokens. There also needs to be meaningful contrast between those three surface tokens. Currently they look like this: 
If we place a minimal button on the darkest of these colors, here's how the hover state looks: 
Clearly there's not much (any) scope to make this color lighter. We could reduce the contrast between the surface backgrounds, which would allow us to also lighten the token in question, but then we lose meaningful contrast between the surfaces to the point where it becomes questionable whether it's worth having them at all: 
Even with this change, the color output is ~ 
Designing with a system always means a degree of back-and-forth and occasional compromise. At this point it’s worth stepping back and asking whether these knock-on changes are worth the cost. For me, system-level coherence typically outweighs the appearance of an individual primitive in a single state. Particularly when the change is so incredibly subtle like this (the difference between If we're still not happy then I’d rather revisit the component design than start forcing changes that risk undermining the system as a whole. |
Contributor
|
It's still a bit dark, but it's an improvement over what exists. And of course we would not want to undermine anything at all. I'm asking in part to explore the territory and for my own education: what options exist with the current design, within the system? Does the backdrop color of the active toggle control have to be the same token as the hover state shown in the example? Not suggesting the following, to be very very clear: but is there room to say: the backdrop of the active state is item 100 on the grayscale? I.e. it would invert that grayscale in darkmode, but otherwise not be tied to any surfaces at all. And yes, the alternative is to revisit the design. Not revert, but see if there's room in the beta territory for small lifts. Here's a togglecontrol playground file, and if we need to, we can go full circle: 
But honestly it feels like there's room for a "track color" token. That has to be light, and such a token doesn't necessarily need a hover color, does it? |
Contributor
It does currently because in order to keep the overall number of tokens low we intentionally 'combined' hover and active states. But even if we separated them the original constraint exists; both colors would need to be visible on the darkest surface color. Given 'active' backgrounds are usually one shade darker than hover backgrounds I don't think separation would help us here. Of course we could use a different token altogether, but that goes against the semantics and is obviously something we should avoid if possible. There is certainly room for a 'Track' token. In fact I think we already have one. The issue is that all the hover/active background tokens are darker than white, so even the proposed design wouldn't be achievable. |
Contributor
|
It does sound like the next step is to explore the territory in Figma then. Happy to huddle there if useful. I feel this is a healthy but important trial for the token system, to be able to account for this challenge. What are the creative ways, back and forth, that will let us improve the control? Because while improved in its reduced optical balance, as-is it's a bit bare compared to other players in the scene. What does ShadCDN do, tokenwise, for example? |
MaggieCabrera
pushed a commit
that referenced
this pull request
Feb 10, 2026
* ToggleGroupControl: use darker bg color for selected item * Update snapshots * CHANGELOG
Contributor
|
Yes I think the next step is probably to explore 'track based' elements and controls together. Things like scrollbars, sliders, progress bars, and (potentially) I agree this is a good trial, but I also think we need to balance impact and effort. It's worth questioning whether this control justifies the (potential) wider implications across the system. |
Contributor
|
I realized that 
Additionally, what if we need to add a disabled state to the
With that in mind, it might be worth considering the approach above 🤔 |
Contributor
|
Some options: 
In context: 
I think I'm using a non existing gray in the above, f0f0f0. Here's one with eaeaea: 
In context: 
I briefly tried with a track backdrop too, but it didn't connect with me: 
|
Contributor
Author
|
@jasmussen , just to make sure — do you have a preference for any new styles to apply to ToggleGroupControl? are there any changes that you'd like to see applied immediately? |
Contributor
I will be mostly AFK today, but otherwise I feel mostly unblocked to help with a PR. But if you have capacity, I'd appreciate your help too. I don't think it has to happen before the beta, but I do think it'd be good to refine this even a bit. The following is improved, but I still get that "unstyled" or "barebones" vibe from it: 
Whereas by mainly finessing the color of inactive icons, especially in the above borderless context, I feel it helps to make it look slightly more intentional: 
I know, this is detail territory, the main change is the color of the icons is now gray, like the text is in text-based segmented controls. |
Contributor
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Tweak the background color of the selected item in the
ToggleGroupControlcomponentWhy?
Add visual emphasis to the selected item
How?
Changed the background color from white to
gray-100Testing Instructions
Screenshots or screencast