Skip to content
This repository was archived by the owner on Jul 7, 2025. It is now read-only.

Conversation

@mperrotti
Copy link
Contributor

No description provided.

@mperrotti mperrotti requested a review from a team as a code owner July 25, 2022 13:38
@mperrotti mperrotti temporarily deployed to github-pages July 25, 2022 13:40 Inactive
Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments, but otherwise these guidelines are great. 👏


### Button label text

Segmented control button labels should be nouns or noun phrases that succinctly describe the choice. Button labels may not wrap to multiple lines.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I still have to fix the wrapping in PCSS.

Copy link
Contributor

@vdepizzol vdepizzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excited for this! Let a couple of quick comments! 🙇

>
<div>

A segmented control is treated like a [toolbar](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/toolbar_role), not a [radio group](https://developer.mozilla.org/en-US/docs/web/accessibility/aria/roles/radiogroup_role).
Copy link
Contributor

@vdepizzol vdepizzol Aug 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the recommendation when a SegmentedControl is used as a tab (akin to TabNav/UnderlineNav behavior)?

What about when a SegmentedControl is a a setting option (emoji skin tone picker)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm bringing this to a11y office hours today: https://github.com/github/accessibility/issues/1556#issuecomment-1204047337

I'll keep you posted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommendation was to not use a segmented control like a tab: https://github.rewatch.com/video/jlbodxdruw5bzc8b-accessibility-office-hours-august-3-2022?t=475.87

cc @simurai

Copy link
Contributor

@simurai simurai Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mperrotti Thanks for bringing this up in the a11y office hour.

The recommendation was to not use a segmented control like a tab

As I understood it, SegmentedControl should behave like a list of buttons where each tab keypress moves to the next button and you can make it active by pressing enter or space.

But switching between "Write/Preview" in the comment box should use what they call a "composite widget". Where only one tab-stop exists and then you would use the arrow keys to toggle between "Write" and "Preview". Basically keep the current TabNav. Also keep using tabs should make it more clear that the two options are mutually exclusive.

<DoDontContainer>
<Do>
<img
src="https://user-images.githubusercontent.com/2313998/180624830-b91217ae-7c79-4698-a71a-a482576e5df9.png"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hover style shown in this image is looking a little squished. Is that also in the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a 32x32 square with a 44px high click area on devices with a
"coarse" pointer: https://primer.style/react/storybook/?path=/story/segmentedcontrol-examples--icons-only

Comment on lines 197 to 198
Segmented controls must have a text label, even if the label is only accessible to screen readers. However, a
visible text label is preferred.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing. Can we list when a visual label is preferred? There are a number of scenarios where the context/location of the SegmentedControl are enough to understand what's it for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we list when a visual label is preferred?

I think we generally prefer visible labels for everything because it adds clarity. cc @alliethu or @ichelsea to keep me correct or add a better explanation.


There are a number of scenarios where the context/location of the SegmentedControl are enough to understand what's it for.

Great point. I'll make sure to directly mention that in this part of the docs

Comment on lines +269 to +270
A segmented control's width can stretch each button evenly to fill the width of the control's parent. This option is
intended to make the segmented control easier to adapt into different layouts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When should we use full-width vs left-aligned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a hard time explaining this because I don't have any specific examples yet. Here are two slightly less vague use-cases:

  • A designer might choose to use a full-width segmented control to make a cleaner layout where elements align to the left and right edges
  • To make sure each button has an even width. If the text of one button is very long, that button would be very wide and have a heavier visual weight than the other buttons.

I think that extra information would just add confusion, and it's better not to try and specify use cases until we have a better example. What do you think?

@mperrotti mperrotti temporarily deployed to github-pages August 3, 2022 18:36 Inactive
@mperrotti mperrotti temporarily deployed to github-pages August 3, 2022 21:12 Inactive
@mperrotti
Copy link
Contributor Author

I have to update the keyboard navigation video based on feedback from @jscholes. During a11y office hours, I learned that a segmented control's keyboard navigation should be like a list (use tabs to focus buttons), not a toolbar (use arrows to focus buttons).

@mperrotti mperrotti temporarily deployed to github-pages August 5, 2022 16:30 Inactive
@mperrotti mperrotti merged commit b51feae into main Aug 5, 2022
@mperrotti mperrotti deleted the mp/segmented-control-guidelines branch August 5, 2022 18:34
@jonrohan jonrohan mentioned this pull request Aug 10, 2022
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants