fix(Button): mark disable usage as invalid

- update stories to document
chanzuckerberg · Jun 6, 2024 · 81b522e · 81b522e
1 parent 8389f35
commit 81b522e
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 42 deletions.
diff --git a/src/components/Button/Button-v2.module.css b/src/components/Button/Button-v2.module.css
@@ -259,57 +259,65 @@
 /**
  * Disabled
  */
+.button--disabled {
+  &.button--primary.button--variant-default,
+  &.button--primary.button--variant-critical,
+  &.button--primary.button--variant-neutral {
+    color: var(--eds-theme-color-text-utility-disabled-primary);
+    border-color: var(--eds-theme-color-background-utility-disabled-medium-emphasis);
+    background-color:  var(--eds-theme-color-background-utility-disabled-medium-emphasis);
+
+    pointer-events: none;
+  }
 
-.button--primary.button--variant-default.button--disabled,
-.button--primary.button--variant-critical.button--disabled,
-.button--primary.button--variant-neutral.button--disabled {
-  color: var(--eds-theme-color-text-utility-disabled-primary);
-  border-color: var(--eds-theme-color-background-utility-disabled-medium-emphasis);
-  background-color:  var(--eds-theme-color-background-utility-disabled-medium-emphasis);
+  &.button--primary.button--variant-inverse {
+    color: var(--eds-theme-color-text-utility-inverse-disabled);
+    /* Using transparent for border color to avoid blending opacity of border and background */
+    border-color: transparent;
+    background-color: var(--eds-theme-color-background-utility-inverse-disabled);
 
-  pointer-events: none;
-}
+    pointer-events: none;
+  }
 
-.button--primary.button--variant-inverse.button--disabled {
-  color: var(--eds-theme-color-text-utility-inverse-disabled);
-  /* Using transparent for border color to avoid blending opacity of border and background */
-  border-color: transparent;
-  background-color: var(--eds-theme-color-background-utility-inverse-disabled);
+  &.button--secondary.button--variant-default,
+  &.button--secondary.button--variant-critical,
+  &.button--secondary.button--variant-neutral {
+    color: var(--eds-theme-color-text-utility-disabled-primary);
+    border-color: var(--eds-theme-color-border-disabled);
+    background-color: var(--eds-theme-color-background-utility-default-no-emphasis);
 
-  pointer-events: none;
-}
+    pointer-events: none;
+  }
 
-.button--secondary.button--variant-default.button--disabled,
-.button--secondary.button--variant-critical.button--disabled,
-.button--secondary.button--variant-neutral.button--disabled {
-  color: var(--eds-theme-color-text-utility-disabled-primary);
-  border-color: var(--eds-theme-color-border-disabled);
-  background-color: var(--eds-theme-color-background-utility-default-no-emphasis);
+  &.button--secondary.button--variant-inverse {
+    color: var(--eds-theme-color-text-utility-inverse-disabled);
+    border-color: var(--eds-theme-color-border-utility-inverse-disabled);
 
-  pointer-events: none;
-}
+    pointer-events: none;
+  }
 
-.button--secondary.button--variant-inverse.button--disabled {
-  color: var(--eds-theme-color-text-utility-inverse-disabled);
-  border-color: var(--eds-theme-color-border-utility-inverse-disabled);
+  &.button--tertiary.button--variant-default,
+  &.button--tertiary.button--variant-critical,
+  &.button--tertiary.button--variant-neutral {
+    color: var(--eds-theme-color-text-utility-disabled-primary);
+    background-color: var(--eds-theme-color-background-utility-default-no-emphasis);
 
-  pointer-events: none;
-}
+    pointer-events: none;
+  }
 
-.button--tertiary.button--variant-default.button--disabled,
-.button--tertiary.button--variant-critical.button--disabled,
-.button--tertiary.button--variant-neutral.button--disabled {
-  color: var(--eds-theme-color-text-utility-disabled-primary);
-  background-color: var(--eds-theme-color-background-utility-default-no-emphasis);
+  &.button--tertiary.button--variant-inverse {
+    color: var(--eds-theme-color-text-utility-disabled-primary);
+    border-color: transparent;
 
-  pointer-events: none;
+    pointer-events: none;
+  }
 }
 
-.button--tertiary.button--variant-inverse.button--disabled {
-  color: var(--eds-theme-color-text-utility-disabled-primary);
-  border-color: transparent;
-
-  pointer-events: none;
+/**
+ * Flag usages of just :disabled as erroneous
+ */
+.button:disabled:not(.button--disabled) {
+  outline: 2px dashed var(--eds-theme-color-border-utility-critical);
 }
 
 /**

diff --git a/src/components/Button/Button-v2.stories.tsx b/src/components/Button/Button-v2.stories.tsx
@@ -36,7 +36,7 @@ type Args = React.ComponentProps<typeof Button>;
 
 export const Default: StoryObj<Args> = {
   args: {
-    children: 'Default',
+    children: 'Button',
   },
 };
 
@@ -65,7 +65,7 @@ export const DefaultRanks: StoryObj<Args> = {
 };
 
 /**
- * Buttons can be disabled for each rank
+ * Buttons can be disabled for each rank using `isDisabled`
  */
 export const Disabled: StoryObj<Args> = {
   args: {
@@ -75,6 +75,18 @@ export const Disabled: StoryObj<Args> = {
   render: DefaultRanks.render,
 };
 
+/**
+ * Since `isDisabled` will set the form's proper disabled state, don't use just `disabled`. This will show a visual error.
+ */
+export const JustDisabledProp: StoryObj<Args> = {
+  args: {
+    ...DefaultRanks.args,
+    disabled: true,
+  },
+  render: DefaultRanks.render,
+  decorators: [(Story) => <div className="p-1">{Story()}</div>],
+};
+
 /**
  * Tertiary buttons can have an additional level of emphasis when stood by themselves. Use this case sparingly.
  */

diff --git a/src/components/Button/Button-v2.tsx b/src/components/Button/Button-v2.tsx
@@ -68,6 +68,8 @@ export type ButtonV2Props<ExtendedElement = unknown> =
 
     /**
      * Whether `Button` is set to disabled state (disables interaction and updates appearance).
+     *
+     * Note: this will set the internal field to `disabled` as well. Prefer this to just setting `disabled`.
      */
     isDisabled?: boolean;
 

diff --git a/src/components/Button/__snapshots__/Button-v2.test.tsx.snap b/src/components/Button/__snapshots__/Button-v2.test.tsx.snap
@@ -45,7 +45,7 @@ exports[`<Button /> (v2) Default story renders snapshot 1`] = `
   <span
     class="button__text"
   >
-    Default
+    Button
   </span>
 </button>
 `;
@@ -328,6 +328,50 @@ exports[`<Button /> (v2) InverseRanks story renders snapshot 1`] = `
 </div>
 `;
 
+exports[`<Button /> (v2) JustDisabledProp story renders snapshot 1`] = `
+<div
+  class="p-1"
+>
+  <div
+    class="flex gap-1"
+  >
+    <button
+      class="button button--layout-none button--primary button--lg button--variant-default"
+      disabled=""
+      type="button"
+    >
+      <span
+        class="button__text"
+      >
+        Primary
+      </span>
+    </button>
+    <button
+      class="button button--layout-none button--secondary button--lg button--variant-default"
+      disabled=""
+      type="button"
+    >
+      <span
+        class="button__text"
+      >
+        Secondary
+      </span>
+    </button>
+    <button
+      class="button button--layout-none button--tertiary button--lg button--variant-default"
+      disabled=""
+      type="button"
+    >
+      <span
+        class="button__text"
+      >
+        Tertiary
+      </span>
+    </button>
+  </div>
+</div>
+`;
+
 exports[`<Button /> (v2) LoadingStates story renders snapshot 1`] = `
 <div
   class="flex items-center gap-1"
-Original file line number
+Diff line change
@@ Expand Up / @@ -68,6 +68,8 @@ export type ButtonV2Props<ExtendedElement = unknown> = @@
         /**
          * Whether `Button` is set to disabled state (disables interaction and updates appearance).
+         *
+         * Note: this will set the internal field to `disabled` as well. Prefer this to just setting `disabled`.
          */
         isDisabled?: boolean;
@@ Expand Down @@