Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ToggleGroupControl design #64439

Open
jameskoster opened this issue Aug 12, 2024 · 19 comments · May be fixed by #68244
Open

Update ToggleGroupControl design #64439

jameskoster opened this issue Aug 12, 2024 · 19 comments · May be fixed by #68244
Labels
Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress

Comments

@jameskoster
Copy link
Contributor

jameskoster commented Aug 12, 2024

When multiple instances of this component appear in quick succession the heavy active state add's a lot of noise and can be overwhelming. Let's explore ways to remedy this in an updated design.

@jameskoster jameskoster converted this from a draft issue Aug 12, 2024
@jameskoster
Copy link
Contributor Author

One potential solution is to outline the active state and apply a gray background to the overall control:

@jameskoster jameskoster added the Needs Design Feedback Needs general design feedback. label Aug 12, 2024
@mirka
Copy link
Member

mirka commented Aug 13, 2024

Fully understanding that this is a difficult design exercise, I'll write down some requirements that ToggleGroupControl has in terms of component states:

  • A (focusable) disabled state for the entire component (currently not implemented at all)
  • A (focusable) disabled state for a single option item (disabled state is implemented, but not focusable disabled)
  • A "de-selectable" variant. This pattern can be seen in Typography controls such as letter case and text decoration.

I guess it's not off the table that we diverge the "standard" ToggleGroupControl from the "de-selectable" which is more like a button group. However, for overall consistency of look & feel, it may make sense to keep the selected item style in line with how we style aria-pressed buttons. Meaning, if we change the selected item style here, we might also want to consider changing it for pressed buttons.

Pressed toggle button in FontSizePicker

@jameskoster
Copy link
Contributor Author

jameskoster commented Aug 13, 2024

I guess it's not off the table that we diverge the "standard" ToggleGroupControl from the "de-selectable" which is more like a button group.

I think that's a good point to discuss. The de-selectable variant is similar to a button group as you pointed out, but also a toolbar. There might be some consolidation to do there.

For me, the base ToggleGroupControl feels different. Semantically it's more like a radio group whereas toolbars/button groups are more flexible in nature, so a different appearance can perhaps be justified.

Potential pathways for controls like Letter case include:

  • Use the base ToggleGroupControl but add a 'None' or 'Default' option.
  • Migrate to ButtonGroup, Toolbar, or something else :)

Here are some mockups for the states you described, plus a couple of others:

Examples

@mirka
Copy link
Member

mirka commented Aug 13, 2024

Great, thank you for integrating all the states. No more blockers from me 👍

For me, the base ToggleGroupControl feels different. Semantically it's more like a radio group whereas toolbars/button groups are more flexible in nature, so a different appearance can perhaps be justified.

Yes, I'm not opposed to this at all. The semantics and behavior are different enough that it's arguably better for them to be styled differently.

Logistically, it would be easiest if we could switch the Letter case and Decoration controls to a standard ToggleGroupControl, and deprecate the isDeselectable variant altogether, since those two are the only use cases in-app.

I'll note that the Letter case and Decoration controls actually have a "None" option which is deselectable. But the deselectable here shouldn't be strictly necessary, because they're part of a ToolsPanel, which provides the "Reset" functionality. The font size control ToggleGroupControl for example is reset through the ToolsPanel dropdown menu. I think we should be allowed to use the same mechanism for Letter case and Decoration, no?

Decoration control with deselectable none option

@jasmussen
Copy link
Contributor

Appreciate the exploration. The main item to solve here is that the dark toggled state can give prominence to the control itself, which in some cases is not desired. This one achieves contrast through the border, though marinating on this, it loses a little bit of the segmented feel, where each option is mutually exclusive to the others. This is a bit more visible in the icon example. There's something to the idea, though, so let's continue to think about how we might thread the needle, perhaps across all the componentry. Maybe it's not possible, that's fine too.

@jameskoster
Copy link
Contributor Author

The main item to solve here is that the dark toggled state can give prominence to the control itself, which in some cases is not desired.

Is this not solved in the mockups above? There may be other options to try – happy to continue exploring / create more mockups – but I don't know that we should let 'perfect' be a blocker to improvement here.

Logistically, it would be easiest if we could switch the Letter case and Decoration controls to a standard ToggleGroupControl, and deprecate the isDeselectable variant altogether

This sounds like a reasonable path forwards to me.

@jasmussen
Copy link
Contributor

It's solved, yes, but on reflection, one past instinct you shared (maybe in chat) was that this perhaps looked a bit too much like an input after all, making this specific issue not a clear win. I'm also thinking that this can be a low-boil set of explorations that consider not just this toggle, but several of them together, with no rush. What do you think?

@jameskoster
Copy link
Contributor Author

That kind of assumes other controls will be changing significantly which seems unlikely in the short term. So it seems a shame for this one detail to hold up any improvements, especially as it's not 100% clear it is actually an issue.

I do agree this isn't high priority though.

@mirka
Copy link
Member

mirka commented Sep 13, 2024

I noticed this bug #62981 which should be fixed sooner rather than later. We can fix the functionality part without a design change, but ideally we should have a design where a focus ring can be shown on a single option item without it being selected (like a radio group that doesn't have a selected item yet).

@jameskoster
Copy link
Contributor Author

Focussed with no selection could look something like this:

focussed no selection

In terms of the individual options, I think they only need focus styles when disabled, right? Otherwise the parent should retain focus?

@ciampo
Copy link
Contributor

ciampo commented Sep 16, 2024

In terms of the individual options, I think they only need focus styles when disabled, right? Otherwise the parent should retain focus?

Imagine having a mix of enabled and disabled option items — wouldn't it be weird if the focus rings moved between the whole group (when an enabled option is focused) and a single option (when it's a disabled option?)

I wonder if we could keep the design proposed above, and show a focus ring also on "enabled" tabs:

Screenshot 2024-09-16 at 12 19 00

We also have a dedicated issue for this specific aspect: #63612

@mirka
Copy link
Member

mirka commented Sep 16, 2024

Yes, I think we're going to need to drop idea of the entire control having a focus ring. When there are no options selected and there's only a focus ring on the entire component, there's not enough affordance for the user to understand what the control does. The first option should get the focus ring, much like a radio group or any other Composite-based control.

@jameskoster
Copy link
Contributor Author

So how would the keyboard interaction work for an instance with no active selection? If the first item receives focus, how would users select it? What happens if they arrowkey right without making a selection – currently this would auto-select the newly 'focused' item, wouldn't that be a bit inconsistent, especially if arrowkey left returns them to the original state and selects.

@ciampo
Copy link
Contributor

ciampo commented Sep 16, 2024

Continuing the analogy with a radio group:

  • like in any composite widget, the "selected" (active) item and the "focused" item don't always coincide; selected items have their radio dot filled, while focused items have some sort of focus ring
  • if a radio group receives focus without a selected item, the first option is usually focused (but not selected); a focus ring is shown;
  • as soon as the user uses the arrow keys to move the focus, an item is selected; also, if the user presses spacebar on a focused (but un-selected) radio item, that item becomes selected.
Kapture.2024-09-16.at.16.25.38.mp4

With this in mind, here is how ToggleGroupControl could work when isDeselectable={false}:

If the first item receives focus, how would users select it?

By pressing spacebar, clicking it, or moving the selection back and forth with arrow keys

What happens if they arrowkey right without making a selection

If the next item is not disabled, pressing the right arrow key would move focus AND select that item.

If the next item is disabled, pressing the right arrow key would move focus to that item, but without making it the new selected item (since it's disabled).

currently this would auto-select the newly 'focused' item, wouldn't that be a bit inconsistent, especially if arrowkey left returns them to the original state and selects.

Not sure I follow entirely this last point, but I think that my replied above should have clarified the behaviour anyway.

In short, it would be very similar (if not identical) to how tab selection works in Tabs when the selectOnMove prop is true.

@mirka what do you think about this proposed behaviour?

@mirka
Copy link
Member

mirka commented Sep 16, 2024

@ciampo I think that makes sense.

In the past, I felt strongly that there should be a clear visual distinction between the deselectable and non-deselectable variants, thus showing a focus ring on the entire component when non-deselectable. But I no longer think that's possible 😕

@jameskoster
Copy link
Contributor Author

Okay, last question is about disabled states. When all options are disabled can you still focus through each item? Arrow left / right would move focus but not select. Does this mean it's not possible for the component itself to be disabled, only individual options?

I think this would cover all the states we've discussed:

Screenshot 2024-09-17 at 13 34 13

@ciampo
Copy link
Contributor

ciampo commented Sep 17, 2024

When all options are disabled can you still focus through each item? Arrow left / right would move focus but not select.

It would be a bit of a weird state, but technically yes — i'd expect each disabled item to be focussable, but not selectable.

Does this mean it's not possible for the component itself to be disabled, only individual options?

I would probably also allow for the whole item to be disabled, as a conceptually separate thing.

I guess that a disabled ToggleGroupControl would still be focussable as a whole, but the user wouldn't be able to focus the individual options — ie. the focus could stay on the whole group, in this case. What do y'all think?

@jasmussen
Copy link
Contributor

I like this one, but there are some details about it I'd love to tinker with to get it just right. I think it's mainly the gray backdrop that isn't fully gelling quite yet. Perhaps after the beta period we can seek out the way to thread the needle fully?

@jameskoster
Copy link
Contributor Author

Please go ahead and tinker! Here's the Figma.

@rinkalpagdar rinkalpagdar linked a pull request Dec 23, 2024 that will close this issue
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress
Projects
Status: Next
Development

Successfully merging a pull request may close this issue.

4 participants