Skip to content

Add selected state to Button #1819

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add selected state to Button #1819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de6c9fa
Add selected state to Button
pksjce Jan 24, 2022
0e721ba
Update snapshots
pksjce Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/content/drafts/Button2.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@mperrotti mperrotti Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@siddharthkp siddharthkp Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The two use cases here:

  1. 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

  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

This is in many cases similar to the active state of the button. Usually used in `ButtonGroup` when like tabs.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. Opens door to misuse of standalone button appearance
  2. A standalone button shouldn't need to be pre-selected

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 don't understand what misuse of this pattern could look like.
How would you restrict to ButtonGroup? By parent class?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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!)

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, 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.


```jsx live
<>
<drafts.Button aria-selected="true">Search</drafts.Button>
<drafts.Button aria-selected="true" variant="danger">
Delete
</drafts.Button>
Comment on lines +75 to +78
Copy link
Member

Choose a reason for hiding this comment

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

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>

</>
```

### Appending an icon

We can place an inside the `Button` in either the leading or the trailing position to enhance the visual context.
Expand Down
5 changes: 5 additions & 0 deletions src/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'] {
Copy link
Member

@siddharthkp siddharthkp Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

background-color: ${get('colors.btn.selectedBg')};
box-shadow: ${get('colors.primer.shadow.inset')};
}

${sx};
`
Expand Down
4 changes: 3 additions & 1 deletion src/Button/ButtonDanger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ const ButtonDanger = styled(ButtonBase)<ButtonBaseProps & SxProp>`
box-shadow: ${get('shadows.btn.danger.focusShadow')};
}

&:active {
&:active,
&.selected,
&[aria-selected='true'] {
color: ${get('colors.btn.danger.selectedText')};
background-color: ${get('colors.btn.danger.selectedBg')};
box-shadow: ${get('shadows.btn.danger.selectedShadow')};
Expand Down
4 changes: 3 additions & 1 deletion src/Button/ButtonInvisible.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ const ButtonInvisible = styled(ButtonBase)<ButtonBaseProps & SxProp>`
&:hover {
background-color: ${get('colors.btn.hoverBg')};
}
&:active {
&:active,
$.selected,
&[aria-selected='true'] {
background-color: ${get('colors.btn.selectedBg')};
}

Expand Down
4 changes: 3 additions & 1 deletion src/Button/ButtonOutline.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ const ButtonOutline = styled(ButtonBase)<ButtonBaseProps & SxProp>`
box-shadow: ${get('shadows.btn.outline.focusShadow')};
}

&:active {
&:active,
$.selected,
&[aria-selected='true'] {
color: ${get('colors.btn.outline.selectedText')};
background-color: ${get('colors.btn.outline.selectedBg')};
box-shadow: ${get('shadows.btn.outline.selectedShadow')};
Expand Down
4 changes: 3 additions & 1 deletion src/Button/ButtonPrimary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ export const ButtonPrimary = styled(ButtonBase)<ButtonBaseProps & SxProp>`
box-shadow: ${get('shadows.btn.primary.focusShadow')};
}

&:active {
&:active,
$.selected,
&[aria-selected='true'] {
background-color: ${get('colors.btn.primary.selectedBg')};
box-shadow: ${get('shadows.btn.primary.selectedShadow')};
}
Expand Down
6 changes: 6 additions & 0 deletions src/Button2/Button2.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ export default {
type: 'radio',
options: ['small', 'medium', 'large']
}
},
'aria-selected': {
defaultValue: false,
control: {
type: 'boolean'
}
}
}
} as Meta
Expand Down
12 changes: 8 additions & 4 deletions src/Button2/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
'&:disabled': {
color: 'primer.fg.disabled',
backgroundColor: 'btn.disabledBg'
},
'&.selected, &[aria-selected="true"]': {
backgroundColor: 'btn.selectedBg',
boxShadow: 'primer.shadow.inset'
}
},
primary: {
Expand All @@ -38,7 +42,7 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
'&:focus:not([disabled])': {
boxShadow: `${theme?.shadows.btn.primary.focusShadow}`
},
'&:active:not([disabled])': {
'&:active:not([disabled]),&.selected, &[aria-selected="true"]': {
backgroundColor: 'btn.primary.selectedBg',
boxShadow: `${theme?.shadows.btn.primary.selectedShadow}`
},
Expand Down Expand Up @@ -70,7 +74,7 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
borderColor: 'btn.danger.focusBorder',
boxShadow: `${theme?.shadows.btn.danger.focusShadow}`
},
'&:active:not([disabled])': {
'&:active:not([disabled]),&.selected, &[aria-selected="true"]': {
color: 'btn.danger.selectedText',
backgroundColor: 'btn.danger.selectedBg',
boxShadow: `${theme?.shadows.btn.danger.selectedShadow}`,
Expand Down Expand Up @@ -101,7 +105,7 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
'&:focus:not([disabled])': {
boxShadow: `${theme?.shadows.btn.focusShadow}`
},
'&:active:not([disabled])': {
'&:active:not([disabled]),&.selected, &[aria-selected="true"]': {
backgroundColor: 'btn.selectedBg'
},
'&:disabled': {
Expand All @@ -128,7 +132,7 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
boxShadow: `${theme?.shadows.btn.outline.focusShadow}`
},

'&:active:not([disabled])': {
'&:active:not([disabled]),&.selected, &[aria-selected="true"]': {
color: 'btn.outline.selectedText',
backgroundColor: 'btn.outline.selectedBg',
boxShadow: `${theme?.shadows.btn.outline.selectedShadow}`,
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/ActionMenu.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ exports[`ActionMenu renders consistently 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c0.selected,
.c0[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

<button
aria-expanded={false}
aria-haspopup="true"
Expand Down
6 changes: 6 additions & 0 deletions src/__tests__/__snapshots__/ActionMenu2.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ exports[`ActionMenu renders consistently 1`] = `
box-shadow: inset 0 0.15em 0.3em rgba(27,31,36,0.15);
}

.c1.selected,
.c1[aria-selected="true"] {
background-color: hsla(220,14%,94%,1);
box-shadow: inset 0 1px 0 rgba(208,215,222,0.2);
}

<div
className="c0"
color="fg.default"
Expand Down
10 changes: 10 additions & 0 deletions src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ exports[`AnchoredOverlay renders consistently 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c1.selected,
.c1[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

<div
className="c0"
color="fg.default"
Expand Down Expand Up @@ -166,6 +171,11 @@ exports[`AnchoredOverlay should render consistently when open 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c1.selected,
.c1[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

.c2 {
background-color: #ffffff;
box-shadow: 0 1px 3px rgba(27,31,36,0.12),0 8px 24px rgba(66,74,83,0.12);
Expand Down
42 changes: 34 additions & 8 deletions src/__tests__/__snapshots__/Button.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ exports[`Button renders consistently 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c0.selected,
.c0[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

<button
className="c0"
/>
Expand Down Expand Up @@ -139,6 +144,11 @@ exports[`Button respects the "disabled" prop 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c0.selected,
.c0[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

<button
className="c0"
disabled={true}
Expand Down Expand Up @@ -203,7 +213,9 @@ exports[`ButtonDanger renders consistently 1`] = `
box-shadow: 0 0 0 3px rgba(164,14,38,0.4);
}

.c0:active {
.c0:active,
.c0.selected,
.c0[aria-selected='true'] {
color: #ffffff;
background-color: hsla(356,72%,44%,1);
box-shadow: inset 0 1px 0 rgba(76,0,20,0.2);
Expand Down Expand Up @@ -278,7 +290,9 @@ exports[`ButtonDanger renders correct disabled styles 1`] = `
box-shadow: 0 0 0 3px rgba(164,14,38,0.4);
}

.c0:active {
.c0:active,
.c0.selected,
.c0[aria-selected='true'] {
color: #ffffff;
background-color: hsla(356,72%,44%,1);
box-shadow: inset 0 1px 0 rgba(76,0,20,0.2);
Expand Down Expand Up @@ -406,7 +420,9 @@ exports[`ButtonInvisible renders consistently 1`] = `
box-shadow: 0 0 0 3px rgba(5,80,174,0.4);
}

.c0:active {
.c0:active,
.c0 $.selected,
.c0[aria-selected='true'] {
color: #ffffff;
background-color: hsla(212,92%,42%,1);
box-shadow: inset 0 1px 0 rgba(0,33,85,0.2);
Expand Down Expand Up @@ -483,7 +499,9 @@ exports[`ButtonInvisible renders correct disabled styles 1`] = `
background-color: #f3f4f6;
}

.c0:active {
.c0:active,
.c0 $.selected,
.c0[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

Expand Down Expand Up @@ -551,7 +569,9 @@ exports[`ButtonOutline renders consistently 1`] = `
box-shadow: 0 0 0 3px rgba(5,80,174,0.4);
}

.c0:active {
.c0:active,
.c0 $.selected,
.c0[aria-selected='true'] {
color: #ffffff;
background-color: hsla(212,92%,42%,1);
box-shadow: inset 0 1px 0 rgba(0,33,85,0.2);
Expand Down Expand Up @@ -627,7 +647,9 @@ exports[`ButtonOutline renders correct disabled styles 1`] = `
box-shadow: 0 0 0 3px rgba(5,80,174,0.4);
}

.c0:active {
.c0:active,
.c0 $.selected,
.c0[aria-selected='true'] {
color: #ffffff;
background-color: hsla(212,92%,42%,1);
box-shadow: inset 0 1px 0 rgba(0,33,85,0.2);
Expand Down Expand Up @@ -702,7 +724,9 @@ exports[`ButtonPrimary renders consistently 1`] = `
box-shadow: 0 0 0 3px rgba(45,164,78,0.4);
}

.c0:active {
.c0:active,
.c0 $.selected,
.c0[aria-selected='true'] {
background-color: hsla(137,55%,36%,1);
box-shadow: inset 0 1px 0 rgba(0,45,17,0.2);
}
Expand Down Expand Up @@ -774,7 +798,9 @@ exports[`ButtonPrimary renders correct disabled styles 1`] = `
box-shadow: 0 0 0 3px rgba(45,164,78,0.4);
}

.c0:active {
.c0:active,
.c0 $.selected,
.c0[aria-selected='true'] {
background-color: hsla(137,55%,36%,1);
box-shadow: inset 0 1px 0 rgba(0,45,17,0.2);
}
Expand Down
22 changes: 18 additions & 4 deletions src/__tests__/__snapshots__/Button2.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ exports[`Button renders consistently 1`] = `
box-shadow: inset 0 0.15em 0.3em rgba(27,31,36,0.15);
}

.c0.selected,
.c0[aria-selected="true"] {
background-color: hsla(220,14%,94%,1);
box-shadow: inset 0 1px 0 rgba(208,215,222,0.2);
}

<button
className="c0"
>
Expand Down Expand Up @@ -179,7 +185,9 @@ exports[`Button styles danger button appropriately 1`] = `
box-shadow: undefined;
}

.c0:active:not([disabled]) {
.c0:active:not([disabled]),
.c0.selected,
.c0[aria-selected="true"] {
color: btn.danger.selectedText;
background-color: btn.danger.selectedBg;
box-shadow: undefined;
Expand Down Expand Up @@ -270,7 +278,9 @@ exports[`Button styles invisible button appropriately 1`] = `
box-shadow: undefined;
}

.c0:active:not([disabled]) {
.c0:active:not([disabled]),
.c0.selected,
.c0[aria-selected="true"] {
background-color: btn.selectedBg;
}

Expand Down Expand Up @@ -374,7 +384,9 @@ exports[`Button styles outline button appropriately 1`] = `
box-shadow: undefined;
}

.c0:active:not([disabled]) {
.c0:active:not([disabled]),
.c0.selected,
.c0[aria-selected="true"] {
color: btn.outline.selectedText;
background-color: btn.outline.selectedBg;
box-shadow: undefined;
Expand Down Expand Up @@ -470,7 +482,9 @@ exports[`Button styles primary button appropriately 1`] = `
box-shadow: undefined;
}

.c0:active:not([disabled]) {
.c0:active:not([disabled]),
.c0.selected,
.c0[aria-selected="true"] {
background-color: btn.primary.selectedBg;
box-shadow: undefined;
}
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/ConfirmationDialog.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ exports[`ConfirmationDialog renders consistently 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c1.selected,
.c1[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

<div
className="c0"
color="fg.default"
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/Dropdown.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ exports[`Dropdown.Button renders consistently 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c0.selected,
.c0[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

.c1 {
border: 4px solid transparent;
margin-left: 12px;
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/DropdownMenu.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ exports[`DropdownMenu renders consistently 1`] = `
border-color: rgba(27,31,36,0.15);
}

.c0.selected,
.c0[aria-selected='true'] {
background-color: hsla(220,14%,94%,1);
}

.c1 {
margin-left: 4px;
}
Expand Down
Loading