Skip to content

Conversation

@jonrohan
Copy link
Member

What are you trying to accomplish?

Fixes #2204

This fixes the selectors to correctly show the line after the component markup changed.

Before

Screen.Recording.2022-08-15.at.1.37.09.PM.mov

After

Screen.Recording.2022-08-15.at.1.35.37.PM.mov

What approach did you choose and why?

For the direct sibling of the currently selected button, I used :has which doesn't have much support yet, but is growing quickly.

In browsers that don't support :has yet, it's not too noticeable.

image

What should reviewers focus on?

I'm open to suggestions on other ways to fix the line issue.

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

@jonrohan jonrohan requested a review from a team as a code owner August 15, 2022 20:46
@jonrohan jonrohan requested a review from simurai August 15, 2022 20:46
@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2022

⚠️ No Changeset found

Latest commit: 9f16206

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jonrohan jonrohan changed the title Fixing the line between buttons SegmentedControl Fixing the line between buttons Aug 15, 2022
@jonrohan jonrohan temporarily deployed to github-pages August 15, 2022 20:52 Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview August 15, 2022 20:53 Inactive
@jonrohan jonrohan temporarily deployed to github-pages August 15, 2022 21:19 Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview August 15, 2022 21:19 Inactive
display: inline-flex;

// stylelint-disable-next-line selector-max-compound-selectors
&:has([aria-current='true']) + li .SegmentedControl-button::before {
Copy link
Contributor

@mperrotti mperrotti Aug 15, 2022

Choose a reason for hiding this comment

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

:has has very low browser support: https://caniuse.com/css-has

Are you intentionally making the dividers a progressive enhancement?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't really think of a way to replace has(), except maybe moving the aria-current='true' to the parent li or some negative margin hack.

Just curious about comment primer/design#276 (comment)

a segmented control's keyboard navigation should be like a list (use tabs to focus buttons)

@mperrotti did you mean to literally use a <ul> and wrap the buttons in <li>s, or did you mean that the focus should just move from one button to the next (like in a list) when pressing tab. If the later, could we also just change the role="toolbar" to role="list" instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed it to semantically be a <ul> in Primer React. Would you propose different markup?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried at first relying on role only, but axe didn't like role="list" > button buttons being direct descendants of role list, and I couldn't apply role=listitem to the button

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, buttons should stay role="button", and we shouldn't have descendants of role="list" that aren't role="listitem"

@github-actions github-actions bot temporarily deployed to Storybook Preview August 16, 2022 19:58 Inactive
@mperrotti
Copy link
Contributor

Now the selected button doesn't have the expected styles. I think this is probably because I'm still on Chrome 104 and I don't have :has yet. (Deployment link)

Screen Shot 2022-08-16 at 7 35 03 PM

I'm going to ask whether aria-current belongs on the li or the button in tomorrow's a11y office hours.

@mperrotti
Copy link
Contributor

I'm in a11y office hours now, and they said that aria-current should go on the button. I'll update this comment with a link to the Rewatch when it's up.

@jonrohan
Copy link
Member Author

closing this in favor of #2208

@jonrohan jonrohan closed this Aug 17, 2022
@jonrohan jonrohan deleted the segmented_control/line branch August 17, 2022 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Segmented control styles are not correct in Storybook

4 participants