Add selected state to Button #1819

pksjce · 2022-01-24T07:36:17Z

Describe your changes here.

I have added selected state as described in https://primer.style/css/components/buttons#selected. I used the same styles as https://github.com/primer/css/blob/e8d5402a26f0e4428cb0cd0694569da3c2ed505e/src/buttons/button.scss#L86

I have implemented this for both Button and Button2.

Closes # (https://github.com/github/primer/issues/639)

Screenshots

Checkout the storybook in

Please provide before/after screenshots for any visual changes

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2022-01-24T07:36:29Z

⚠️ No Changeset found

Latest commit: 0e721ba

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

github-actions · 2022-01-24T07:40:56Z

size-limit report 📦

Path	Size
dist/browser.esm.js	61.3 KB (+0.08% 🔺)
dist/browser.umd.js	61.67 KB (+0.07% 🔺)

rezrah · 2022-01-24T11:14:53Z

docs/content/drafts/Button2.mdx

@@ -65,6 +65,20 @@ The `invisible` variant of `Button` indicates that the action is a low priority
 </>
 ```

+### Selected button
+
+`Button` can be set to in a "selected" state in two ways. You can use a `selected` class or set `aria-selected ="true"`.


Is it worth adding a selected prop that handles both styling and setting aria attributes? May make consumer-side implementation easier. Also, I don't usually ask for users to set classes elsewhere to enable features, so could this be confusing?

Same! this was the exact confusion I was trying to discuss on slack as well. prop or class? Class is just not how we'd do this in react

Should it bearia-pressed instead? 🤔

The aria-selected attribute indicates the current "selected" state for gridcell, option, row and tab roles.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-selected

The aria-pressed attribute indicates the current "pressed" state of a toggle button.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed

cc @alliethu - what do you think the correct ARIA attribute might be?

Agreed that we should do it with a prop instead of a class or attribute selector.

This came up today in the Primer patterns working session. We decided this should be purely visual and not set any ARIA attributes.

In the case where a button controls a dropdown, it would need to be passed aria-haspopup and aria-expanded attributes. These attributes would be passed by the component that renders the dropdown button and menu.

cc @langermank

Two questions here

How do we set the pressed styles in this case?

How do we support the toggles in the tabs use case in the issue Button state selected #1783 ?

I'm thinking we need to implement aria-pressed styles and scrap the selected prop.

The two use cases here:

Sounds like Button state selected #1783 is using Button as a "toggle button", which isn't just a visual update but also a semantic change.

From MDN (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed):

Adding aria-pressed to an element with a role of button turns the button into a toggle button. The aria-pressed attribute is only relevant for toggle buttons. It represents the button's current "pressed" state.

It looks like it makes sense to add aria-pressed for toggle button. cc @alliethu for confirmation

For buttons that open a Menu, aria-pressed doens't make sense. Instead, we add the aria attributes aria-haspopup="true" aria-expanded="true", we can modify button styles if those are present OR to keep the two concerns independent, we can pass a pressed or active prop or className (not aria-pressed) to the Button from the Menu components.

Got it, so @mperrotti @siddharthkp I'm adding a pressed prop to the component and we can handle the css based on that. This will address both the above use cases.

rezrah · 2022-01-24T11:18:27Z

docs/content/drafts/Button2.mdx

+### Selected button
+
+`Button` can be set to in a "selected" state in two ways. You can use a `selected` class or set `aria-selected ="true"`.
+This is in many cases similar to the active state of the button. Usually used in `ButtonGroup` when like tabs.


Should this capability be restricted to ButtonGroup? I can see two risks with enabling this for individual buttons:

Opens door to misuse of standalone button appearance

A standalone button shouldn't need to be pre-selected

🤔 I don't understand what misuse of this pattern could look like.
How would you restrict to ButtonGroup? By parent class?

We talked about this in the Primer React planning meeting.

I don't think we want to restrict it to ButtonGroup buttons, especially because we use the "selected" styling when a button controls a dropdown and the dropdown is open.

Kapture.2022-01-25.at.13.32.30.mp4

I think what we decided in the meeting is to utilize the active state for dropdowns (this is all forward thinking, not talking about existing functionality) and restrict aria-pressed to a future new Segmented Control component (a more specific version of a ButtonGroup). The confusing piece of this is that we are simultaneously updating an existing Button component while refactoring a future Button component.

@pksjce I may have missed this, but do you have the use case that prompted this ask to add selected? We may need to continue supporting it in the original Button while we redesign the future Button.. but my recommendation there will depend on the use case for adding it now.

@langermank - this was the original issue where this came up #1783

Thanks @rezrah.. that's an interesting use case 🤔 I still think this would be a SegmentedControl though I imagined that to be two/three options max not six 😆 I guess in that case to unblock them use the aria-selected styles from Primer CSS but map it to aria-pressed? But I would only add this to the existing Button component, not the refactor.

We are planning to deprecate the existing component as soon as possible. I think for parity we'd need to have it in the new one as well? The new component is not a refactor in terms of functionality I feel, only the API. Once you are done with primer/css#1874, we'd have to work on absorbing it into primer-react.

@pksjce right, thanks for pointing that out. How do you see the new Button API changes rolling into PRC? In CSS and PVC, I think we'll likely end up creating a new component similar to how you've handled Button2. If you follow that pattern in PRC, you would end up with Button3 😆 Although, the markup in Button2 is almost identical to where we're going with the Button API changes, so perhaps it wouldn't be a big deal to wrap it into Button2 once Button is deprecated (this is getting a little confusing!)

Yes, I'm not planning to have a Button3 for sure. I'd just update Button2 which would become the only Button and release it going forward. I don't think there'd be any major API changes. If any breaking then we'd go for a major version change I believe.

siddharthkp · 2022-02-07T09:08:46Z

docs/content/drafts/Button2.mdx

+  <drafts.Button aria-selected="true">Search</drafts.Button>
+  <drafts.Button aria-selected="true" variant="danger">
+    Delete
+  </drafts.Button>


Would be nice to have examples for both ways of doing it :)

Suggested change

<drafts.Button aria-selected="true">Search</drafts.Button>

<drafts.Button aria-selected="true" variant="danger">

Delete

</drafts.Button>

<drafts.Button aria-selected="true">Search</drafts.Button>

<drafts.Button className="selected" variant="danger">

Delete

</drafts.Button>

siddharthkp · 2022-02-07T09:09:50Z

src/Button/Button.tsx

@@ -31,6 +31,11 @@ const Button = styled(ButtonBase)<ButtonBaseProps & SxProp>`
    background-color: ${get('colors.btn.bg')};
    border-color: ${get('colors.btn.border')};
  }
+  &.selected,
+  &[aria-selected='true'] {


👍 good call on the =true! I've done the mistake of just putting aria-selected in the selector and catching =false coz react 😅

Add selected state to Button

de6c9fa

pksjce requested review from a team and siddharthkp January 24, 2022 07:36

github-actions bot deployed to Preview January 24, 2022 07:37 View deployment

Update snapshots

0e721ba

pksjce force-pushed the pk/button-selected branch from 1957ea4 to 0e721ba Compare January 24, 2022 10:07

github-actions bot deployed to Preview January 24, 2022 10:07 View deployment

rezrah reviewed Jan 24, 2022

View reviewed changes

siddharthkp reviewed Feb 7, 2022

View reviewed changes

siddharthkp approved these changes Feb 7, 2022

View reviewed changes

pksjce closed this Feb 14, 2022

joshblack deleted the pk/button-selected branch January 19, 2023 16:47

Add selected state to Button #1819

Add selected state to Button #1819

Uh oh!

Conversation

pksjce commented Jan 24, 2022

Screenshots

Merge checklist

Uh oh!

changeset-bot bot commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mperrotti Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

changeset-bot bot commented Jan 24, 2022 •

edited

Loading

github-actions bot commented Jan 24, 2022 •

edited

Loading

mperrotti Jan 25, 2022 •

edited

Loading

siddharthkp Feb 2, 2022 •

edited

Loading

siddharthkp Feb 7, 2022 •

edited

Loading