From 89db30f7c313b3968c6fb05f139d57b07ebe0da5 Mon Sep 17 00:00:00 2001
From: Mateusz Romek <46934668+MateuszRomek@users.noreply.github.com>
Date: Thu, 8 Sep 2022 12:57:16 +0200
Subject: [PATCH] Remove disabled attribute from button (#412)

* Remove disabled attribute from button, add aria-disabled, prevent onClick if disabled is true

* merge unit tests
---
 .../src/components/Button/Button.module.scss  | 106 +++++++++---------
 .../src/components/Button/Button.spec.tsx     |  14 ++-
 .../src/components/Button/Button.tsx          |   4 +-
 3 files changed, 68 insertions(+), 56 deletions(-)

diff --git a/packages/react-components/src/components/Button/Button.module.scss b/packages/react-components/src/components/Button/Button.module.scss
index 6ac99b06f..f81a0b645 100644
--- a/packages/react-components/src/components/Button/Button.module.scss
+++ b/packages/react-components/src/components/Button/Button.module.scss
@@ -1,24 +1,24 @@
 $base-class: 'btn';
 
 .#{$base-class} {
+  display: inline-flex;
+  position: relative;
   align-items: center;
-  border-radius: 4px;
-  border-style: solid;
+  justify-content: center;
+  transition-duration: 200ms;
+  transition-property: opacity, border, color, background-color, box-shadow;
+  transition-timing-function: cubic-bezier(0.64, 0, 0.35, 1);
   border-width: 1px;
+  border-style: solid;
+  border-radius: 4px;
   cursor: pointer;
-  display: inline-flex;
-  font-size: 15px;
-  font-weight: 600;
-  height: 36px;
-  justify-content: center;
-  min-width: 36px;
   padding: 8px 16px;
-  position: relative;
+  min-width: 36px;
+  height: 36px;
   text-align: center;
   text-decoration: none;
-  transition-duration: 200ms;
-  transition-property: opacity, border, color, background-color, box-shadow;
-  transition-timing-function: cubic-bezier(0.64, 0, 0.35, 1);
+  font-size: 15px;
+  font-weight: 600;
   user-select: none;
 
   &:focus,
@@ -28,7 +28,7 @@ $base-class: 'btn';
   }
 
   &:focus {
-    box-shadow: 0 0 1px 2px rgba(var(--color-action-default-rgb), 0.7);
+    box-shadow: 0 0 1px 2px rgb(var(--color-action-default-rgb) 0.7);
   }
 
   &--disabled {
@@ -49,8 +49,8 @@ $base-class: 'btn';
   }
 
   &--basic {
-    background-color: var(--surface-basic-default);
     border-color: var(--color-action-default);
+    background-color: var(--surface-basic-default);
     color: var(--color-action-default);
 
     &:hover {
@@ -63,82 +63,82 @@ $base-class: 'btn';
       color: var(--color-action-hover);
     }
 
-    &[disabled] {
-      background-color: inherit;
+    &[aria-disabled='true'] {
       border-color: var(--color-action-disabled);
+      background-color: inherit;
       color: var(--color-action-disabled);
     }
   }
 
   &--primary {
-    background-color: var(--color-action-default);
     border-color: var(--color-action-default);
+    background-color: var(--color-action-default);
     color: var(--content-invert-default);
 
     &:hover {
-      background-color: var(--color-action-hover);
       border-color: var(--color-action-hover);
+      background-color: var(--color-action-hover);
     }
 
     &:active {
-      background-color: var(--color-action-active);
       border-color: var(--color-action-active);
+      background-color: var(--color-action-active);
     }
 
-    &[disabled] {
-      background-color: var(--color-action-disabled);
+    &[aria-disabled='true'] {
       border-color: var(--color-action-disabled);
+      background-color: var(--color-action-disabled);
     }
   }
 
   &--secondary {
-    background-color: var(--surface-basic-default);
     border-color: var(--surface-secondary-default);
+    background-color: var(--surface-basic-default);
     color: var(--content-default);
 
     &:hover {
-      background-color: var(--surface-basic-hover);
       border-color: var(--surface-secondary-hover);
+      background-color: var(--surface-basic-hover);
     }
 
     &:active {
-      background-color: var(--surface-feedback-info);
       border-color: var(--color-action-default);
+      background-color: var(--surface-feedback-info);
       color: var(--color-action-default);
     }
 
-    &[disabled] {
-      background-color: inherit;
+    &[aria-disabled='true'] {
       border-color: var(--border-disabled);
+      background-color: inherit;
       color: var(--content-disabled);
     }
   }
 
   &--destructive {
-    background-color: var(--color-negative-default);
     border-color: var(--color-negative-default);
+    background-color: var(--color-negative-default);
     color: var(--content-invert-default);
 
     &:hover {
-      background-color: var(--color-negative-hover);
       border-color: var(--color-negative-hover);
+      background-color: var(--color-negative-hover);
     }
 
     &:active {
-      background-color: var(--color-negative-active);
       border-color: var(--color-negative-active);
+      background-color: var(--color-negative-active);
     }
 
-    &[disabled] {
-      background-color: var(--color-negative-disabled);
+    &[aria-disabled='true'] {
       border-color: var(--color-negative-disabled);
+      background-color: var(--color-negative-disabled);
     }
   }
 
   &--compact {
-    height: 32px;
-    min-width: 32px;
     padding: 6px 16px;
+    min-width: 32px;
+    height: 32px;
 
     &.#{$base-class}--icon-only {
       padding: 4px;
@@ -146,9 +146,9 @@ $base-class: 'btn';
   }
 
   &--large {
-    height: 42px;
-    min-width: 42px;
     padding: 11px 16px;
+    min-width: 42px;
+    height: 42px;
 
     &.#{$base-class}--icon-only {
       padding: 9px;
@@ -158,15 +158,15 @@ $base-class: 'btn';
   &--text,
   &--plain,
   &--plain-light {
-    background-color: transparent;
     border-color: transparent;
+    background-color: transparent;
     color: var(--color-action-default);
 
     &:hover {
       color: var(--color-action-hover);
     }
 
-    &[disabled] {
+    &[aria-disabled='true'] {
       color: var(--color-action-disabled);
     }
   }
@@ -182,9 +182,9 @@ $base-class: 'btn';
   &--plain,
   &--plain-light {
     border: 0;
-    height: auto;
-    min-width: auto;
     padding: 0;
+    min-width: auto;
+    height: auto;
 
     &:hover,
     &:focus {
@@ -198,34 +198,34 @@ $base-class: 'btn';
   }
 
   &--loading {
-    cursor: progress;
     transition: all 0.2s cubic-bezier(0.64, 0, 0.35, 1);
+    cursor: progress;
 
     > * {
       opacity: 0;
     }
 
     .#{$base-class}__loader {
-      align-items: center;
       box-sizing: border-box;
       display: flex;
-      height: 100%;
-      justify-content: center;
+      position: absolute;
+      top: 0;
       left: 0;
+      align-items: center;
+      justify-content: center;
       opacity: 1;
       padding: inherit;
-      position: absolute;
-      top: 0;
       width: 100%;
+      height: 100%;
 
       > * {
-        color: inherit;
-        font-weight: inherit;
         margin: 0 5px;
         overflow: hidden;
         text-align: left;
         text-overflow: ellipsis;
         white-space: nowrap;
+        color: inherit;
+        font-weight: inherit;
       }
     }
   }
@@ -236,22 +236,22 @@ $base-class: 'btn';
   }
 
   &__icon {
-    color: inherit;
     display: inline-block;
+    width: 20px;
     height: 20px;
     line-height: 1;
-    width: 20px;
+    color: inherit;
 
     &--left {
-      margin-left: -4px;
-      margin-right: 5px;
       order: -1;
+      margin-right: 5px;
+      margin-left: -4px;
     }
 
     &--right {
-      margin-left: 5px;
-      margin-right: -4px;
       order: 1;
+      margin-right: -4px;
+      margin-left: 5px;
     }
   }
 }
diff --git a/packages/react-components/src/components/Button/Button.spec.tsx b/packages/react-components/src/components/Button/Button.spec.tsx
index 8b01239a7..01fad9732 100644
--- a/packages/react-components/src/components/Button/Button.spec.tsx
+++ b/packages/react-components/src/components/Button/Button.spec.tsx
@@ -1,6 +1,6 @@
 import * as React from 'react';
 
-import { render } from 'test-utils';
+import { render, fireEvent, vi } from 'test-utils';
 import { Icon } from '../Icon';
 
 import { Button } from './Button';
@@ -9,7 +9,6 @@ import styles from './Button.module.scss';
 describe('<Button> component', () => {
   function renderButton(props = {}) {
     const result = render(<Button {...props} />);
-
     return {
       ...result,
       btnEl: result.container.firstChild,
@@ -107,4 +106,15 @@ describe('<Button> component', () => {
     expect((btnEl as HTMLElement).tagName).toBe('BUTTON');
     expect((linkEl as HTMLElement).tagName).toBe('A');
   });
+
+  it('should set aria-disabled if we pass disabled prop and should not fire onClick callback if disabled is true', () => {
+    const onClick = vi.fn();
+    const { btnEl } = renderButton({ disabled: true, onClick });
+
+    expect(btnEl).toHaveAttribute('aria-disabled', 'true');
+
+    fireEvent.click(btnEl as Element);
+
+    expect(onClick).not.toHaveBeenCalled();
+  });
 });
diff --git a/packages/react-components/src/components/Button/Button.tsx b/packages/react-components/src/components/Button/Button.tsx
index eaac29b26..97fe2f043 100644
--- a/packages/react-components/src/components/Button/Button.tsx
+++ b/packages/react-components/src/components/Button/Button.tsx
@@ -41,6 +41,7 @@ export const Button: React.FC<ButtonProps> = ({
   className,
   children,
   href,
+  onClick,
   ...props
 }) => {
   const isDisabled = loading || disabled;
@@ -63,9 +64,10 @@ export const Button: React.FC<ButtonProps> = ({
   return (
     <Component
       className={mergedClassNames}
-      disabled={isDisabled}
+      aria-disabled={isDisabled}
       type={type}
       href={isDisabled ? undefined : href}
+      onClick={isDisabled ? undefined : onClick}
       {...props}
     >
       {loading && (