Adds 'inactive' state to buttons #3812

mperrotti · 2023-10-12T17:30:08Z

Relates to https://github.com/github/primer/issues/2676

This came from the work we did on graceful degradation patterns: https://primer.style/design/ui-patterns/degraded-experiences#indicating-a-button-is-non-functional-without-disabling-it

Changelog

New

Adds inactive prop to buttons

Changed

No changes

Removed

No removals

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Just confirm that the "inactive" button visually looks the same as a "disabled" button.

I've already reviewed this with a11y designers.

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 · 2023-10-12T17:30:11Z

🦋 Changeset detected

Latest commit: cdfaebe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

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

github-actions · 2023-10-12T17:33:38Z

size-limit report 📦

Path	Size
dist/browser.esm.js	104.33 KB (+0.1% 🔺)
dist/browser.umd.js	104.85 KB (+0.1% 🔺)

langermank · 2023-10-12T18:17:06Z

src/Button/ButtonBase.tsx

@@ -21,6 +21,7 @@ const ButtonBase = forwardRef(
      size = 'medium',
      alignContent = 'center',
      block = false,
+      inactive,


Curious, what if we just continue to use disabled as a prop but change it to use aria-disabled instead of disabled across the board?

Is this primarily used for the loading state? So it looked disabled but it can be focused.. can it also still be clicked and follow through with the on click action?

I think there are still rare cases where we want a real disabled button.

The loading state is one way we can use this pattern, but it's mostly to support "degraded" buttons: https://primer.style/design/ui-patterns/degraded-experiences#indicating-a-button-is-non-functional-without-disabling-it

Curious, what if we just continue to use disabled as a prop but change it to use aria-disabled instead of disabled across the board?

I like the general idea of that, but it should come with handling click events with a useful message, which I'm not confident would happen :(

I think there are still rare cases where we want a real disabled button.

Also fair!

mperrotti · 2023-10-12T18:53:41Z

The a11y tests are failing because the inactive button doesn't meet the contrast requirements.

However, I don't think this needs to meet the contrast requirements because it's treated like a disabled control. Is there a way to explicitly skip the color contrast check for this test?

joshblack · 2023-10-13T18:01:06Z

@mperrotti if we're intending for inactive buttons to convey information/receive interactions, would it fall under this scenario for color contrast even if it's considered disabled? https://github.com/github/primer/discussions/652#discussioncomment-7084814

Also wanted to ask, if it's still allowing for interaction is aria-disabled appropriate? If I understand right, aria-disabled="true" would indicate that the control is not operable

mperrotti · 2023-10-13T20:11:59Z

@joshblack - when I brought this idea to the a11y team last quarter, they recommended aria-disabled. The experience would be pretty much the same for AT and keyboard users - the main difference being the aria-disabled attribute.

I double-checked my approach with @ericwbailey this afternoon, and he thinks it's ok if "inactive" buttons don't meet the contrast minimum as long as there's some other indication on the page (such as a global banner) that informs the users that things are broken.

Also, I'm not sure if this falls into the same category as the loading state button because the button itself isn't conveying any information. If somebody with low vision hits a submit button and it "disappears" for them when it goes into the loading state, they might think they broke something.

src/Button/styles.ts

pksjce

aria-disabled can itself be a state or it can have value true/false. If we choose to add the value, then we must use style selectors that include the value. If we choose to remove the attribute when not inactive, then the current style selectors will suffice.

This can be seen in the storybooks esp Playground where all variants of the Button are inactive.

src/Button/ButtonBase.tsx

pksjce · 2023-10-16T05:56:04Z

src/Button/styles.ts

@@ -7,15 +7,15 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
      color: 'btn.text',
      backgroundColor: 'btn.bg',
      boxShadow: `${theme?.shadows.btn.shadow}, ${theme?.shadows.btn.insetShadow}`,
-      '&:hover:not([disabled])': {
+      '&:hover:not([disabled]):not([aria-disabled])': {


Or these selectors should be [aria-disabled="true"]?

mperrotti · 2023-11-30T15:04:44Z

aria-disabled were leftover from an earlier iteration of the design, but now I've removed them.

…inactive-button

primer-integration · 2024-12-18T20:06:31Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/355822

primer-integration · 2024-12-18T20:22:26Z

🟢 golden-jobs completed with status success.

mperrotti added 3 commits October 12, 2023 13:18

adds inactive state to button components

a45f5d5

update snapshots

557b012

adds changeset

f3f0f40

mperrotti requested review from a team and siddharthkp October 12, 2023 17:30

Merge branch 'main' into mp/inactive-button

749e57a

mperrotti had a problem deploying to github-pages October 12, 2023 17:37 — with GitHub Actions Failure

mperrotti added the update snapshots label Oct 12, 2023

test(vrt): update snapshots

16f34a0

github-actions bot removed the update snapshots label Oct 12, 2023

primer bot temporarily deployed to github-pages October 12, 2023 17:56 Inactive

github-actions bot temporarily deployed to storybook-preview-3812 October 12, 2023 17:56 Inactive

langermank reviewed Oct 12, 2023

View reviewed changes

Merge branch 'main' into mp/inactive-button

032dc7b

mperrotti temporarily deployed to github-pages October 12, 2023 18:29 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3812 October 12, 2023 18:29 Inactive

mperrotti mentioned this pull request Oct 12, 2023

Adds 'inactive' state to buttons primer/view_components#2283

Merged

10 tasks

Merge branch 'main' into mp/inactive-button

78d638e

mperrotti temporarily deployed to github-pages October 13, 2023 15:04 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3812 October 13, 2023 15:04 Inactive

Merge branch 'main' into mp/inactive-button

4f64d94

mperrotti temporarily deployed to github-pages October 13, 2023 20:18 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3812 October 13, 2023 20:18 Inactive

pksjce reviewed Oct 16, 2023

View reviewed changes

src/Button/styles.ts Show resolved Hide resolved

pksjce reviewed Oct 16, 2023

View reviewed changes

github-actions bot temporarily deployed to storybook-preview-3812 November 30, 2023 14:48 Inactive

mperrotti mentioned this pull request Nov 30, 2023

Add stories to show Tooltip and Dialog usage in Inactive Buttons #3990

Closed

mperrotti added 2 commits November 30, 2023 16:18

Merge branch 'main' of github.com:primer/react into mp/inactive-button

8d3cd3a

preserves border on inactive buttons

fc9d713

mperrotti temporarily deployed to github-pages November 30, 2023 21:36 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3812 November 30, 2023 21:37 Inactive

langermank approved these changes Nov 30, 2023

View reviewed changes

mperrotti added the update snapshots label Dec 1, 2023

test(vrt): update snapshots

737b6ca

github-actions bot removed the update snapshots label Dec 1, 2023

primer bot temporarily deployed to github-pages December 1, 2023 16:22 Inactive

github-actions bot temporarily deployed to storybook-preview-3812 December 1, 2023 16:22 Inactive

pksjce approved these changes Dec 4, 2023

View reviewed changes

mperrotti added 2 commits December 5, 2023 11:08

Merge branch 'main' of github.com:primer/react into mp/inactive-button

05e8b53

Merge branch 'mp/inactive-button' of github.com:primer/react into mp/…

2196c4a

…inactive-button

mperrotti temporarily deployed to github-pages December 5, 2023 16:13 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3812 December 5, 2023 16:14 Inactive

Merge branch 'main' into mp/inactive-button

cdfaebe

mperrotti enabled auto-merge December 5, 2023 18:42

mperrotti deployed to github-pages December 5, 2023 18:46 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3812 December 5, 2023 18:46 View deployment

mperrotti added this pull request to the merge queue Dec 5, 2023

Merged via the queue into main with commit 3f82a1c Dec 5, 2023
28 checks passed

mperrotti deleted the mp/inactive-button branch December 5, 2023 18:58

primer bot mentioned this pull request Dec 5, 2023

Release tracking #4016

Merged

github-actions bot added the integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh label Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds 'inactive' state to buttons #3812

Adds 'inactive' state to buttons #3812

mperrotti commented Oct 12, 2023 •

edited

Loading

changeset-bot bot commented Oct 12, 2023 •

edited

Loading

github-actions bot commented Oct 12, 2023 •

edited

Loading

langermank Oct 12, 2023

mperrotti Oct 12, 2023

siddharthkp Oct 17, 2023

mperrotti commented Oct 12, 2023

joshblack commented Oct 13, 2023

mperrotti commented Oct 13, 2023

pksjce left a comment

pksjce Oct 16, 2023

mperrotti commented Nov 30, 2023

primer-integration bot commented Dec 18, 2024

primer-integration bot commented Dec 18, 2024

Adds 'inactive' state to buttons #3812

Adds 'inactive' state to buttons #3812

Conversation

mperrotti commented Oct 12, 2023 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Oct 12, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 12, 2023 • edited Loading

size-limit report 📦

langermank Oct 12, 2023

Choose a reason for hiding this comment

mperrotti Oct 12, 2023

Choose a reason for hiding this comment

siddharthkp Oct 17, 2023

Choose a reason for hiding this comment

mperrotti commented Oct 12, 2023

joshblack commented Oct 13, 2023

mperrotti commented Oct 13, 2023

pksjce left a comment

Choose a reason for hiding this comment

pksjce Oct 16, 2023

Choose a reason for hiding this comment

mperrotti commented Nov 30, 2023

primer-integration bot commented Dec 18, 2024

primer-integration bot commented Dec 18, 2024

mperrotti commented Oct 12, 2023 •

edited

Loading

changeset-bot bot commented Oct 12, 2023 •

edited

Loading

github-actions bot commented Oct 12, 2023 •

edited

Loading