Button refactor: use shared functional component for IconButton and B…

…utton (#133)

* use shared internal button for Button and IconButton

* comment

* update disabled styles and tests for IconButton

src/components/Alert/Alert.tsx

@@ -24,7 +24,7 @@ import CloseIcon from "@vector-im/compound-design-tokens/icons/close.svg";
 
 import styles from "./Alert.module.css";
 import { Text } from "../Typography/Text";
-import { IconButton } from "../IconButton/IconButton";
+import { IconButton } from "../Button";
 
 type AlertProps = {
   /**

src/components/Alert/__snapshots__/Alert.test.tsx.snap

@@ -72,9 +72,10 @@ exports[`Alert > renders actions 1`] = `
     </div>
     <button
       aria-label="Close"
-      class="_icon-button_1d4528 _close_6cfd7b"
+      class="_icon-button_e3253e _close_6cfd7b"
       role="button"
       style="--cpd-icon-button-size: 32px;"
+      tabindex="0"
     >
       <svg
         class="cpd-icon"
@@ -134,9 +135,10 @@ exports[`Alert > renders critical 1`] = `
     </div>
     <button
       aria-label="Close"
-      class="_icon-button_1d4528 _close_6cfd7b"
+      class="_icon-button_e3253e _close_6cfd7b"
       role="button"
       style="--cpd-icon-button-size: 32px;"
+      tabindex="0"
     >
       <svg
         class="cpd-icon"
@@ -206,9 +208,10 @@ exports[`Alert > renders info 1`] = `
     </div>
     <button
       aria-label="Close"
-      class="_icon-button_1d4528 _close_6cfd7b"
+      class="_icon-button_e3253e _close_6cfd7b"
       role="button"
       style="--cpd-icon-button-size: 32px;"
+      tabindex="0"
     >
       <svg
         class="cpd-icon"
@@ -268,9 +271,10 @@ exports[`Alert > renders success 1`] = `
     </div>
     <button
       aria-label="Close"
-      class="_icon-button_1d4528 _close_6cfd7b"
+      class="_icon-button_e3253e _close_6cfd7b"
       role="button"
       style="--cpd-icon-button-size: 32px;"
+      tabindex="0"
     >
       <svg
         class="cpd-icon"

src/components/Button/Button.tsx

@@ -23,6 +23,7 @@ import React, {
   Ref,
 } from "react";
 import styles from "./Button.module.css";
+import { UnstyledButton, UnstyledButtonPropsFor } from "./UnstyledButton";
 
 interface ButtonComponent {
   // With the explicit `as` prop
@@ -46,18 +47,10 @@ type ButtonOwnProps = PropsWithChildren<{
    * An icon to display within the button.
    */
   Icon?: ComponentType<React.SVGAttributes<SVGElement>>;
-  /**
-   * Note that disabled attribute is not added to buttons, so that disabled buttons are discoverable by keyboard.
-   * `aria-disabled` attribute is used to indicate button is disabled.
-   * Event handlers are not passed to disabled buttons (onClick, onSubmit).
-   */
-  disabled?: boolean;
 }>;
 
 type ButtonPropsFor<C extends React.ElementType> = ButtonOwnProps &
-  Omit<React.ComponentPropsWithoutRef<C>, keyof ButtonOwnProps | "as"> & {
-    ref?: React.Ref<React.ComponentRef<C>>;
-  };
+  UnstyledButtonPropsFor<C>;
 
 /**
  * A button component that can be transformed into a link, but keep the button
@@ -78,32 +71,20 @@ export const Button = forwardRef(function Button<
   }: ButtonPropsFor<C> & { as?: C },
   ref: ForwardedRef<C>,
 ): React.ReactElement {
-  const Component = as || ("button" as const);
   const classes = classNames(styles.button, className, {
     [styles["has-icon"]]: Icon,
   });
 
-  const { onClick, onSubmit, ...restProps } = props;
-  const eventHandlers = disabled
-    ? {}
-    : {
-        onClick,
-        onSubmit,
-      };
-
   return (
-    <Component
-      {...restProps}
+    <UnstyledButton
+      {...props}
+      as={as || ("button" as const)}
       ref={ref as Ref<C>}
       className={classes}
-      data-kind={kind}
       data-size={size}
-      // All elements roles should be overriden at the exceptions of anchors
-      // We want them to behave like links but look like buttons
-      role={as === "a" ? "link" : "button"}
+      data-kind={kind}
       tabIndex={0}
-      {...eventHandlers}
-      aria-disabled={disabled}
+      disabled={disabled}
     >
       {Icon && (
         <Icon
@@ -114,6 +95,6 @@ export const Button = forwardRef(function Button<
         />
       )}
       {children}
-    </Component>
+    </UnstyledButton>
   );
 }) as ButtonComponent;

src/components/IconButton/IconButton.module.css → src/components/Button/IconButton/IconButton.module.css

@@ -33,7 +33,7 @@ limitations under the License.
   line-height: 0;
 }
 
-.icon-button:disabled {
+.icon-button[aria-disabled="true"] {
   color: var(--cpd-color-icon-disabled);
   cursor: not-allowed;
 }
@@ -70,19 +70,19 @@ limitations under the License.
  */
 
 @media (hover) {
-  .icon-button:not(:disabled):hover {
+  .icon-button:not([aria-disabled="true"]):hover {
     color: var(--cpd-color-icon-primary);
     background: var(--cpd-color-bg-subtle-primary);
   }
 }
 
-.icon-button:not(:disabled):active {
+.icon-button:not([aria-disabled="true"]):active {
   color: var(--cpd-color-icon-primary);
   background: var(--cpd-color-bg-subtle-primary);
 }
 
 @media (hover) {
-  .icon-button[data-indicator]:is(:hover)::before {
+  .icon-button:not([aria-disabled="true"])[data-indicator]:is(:hover)::before {
     /* Same colour as the background */
     border: var(--cpd-icon-button-indicator-border-size) solid
       var(--cpd-color-bg-subtle-primary);
@@ -91,7 +91,7 @@ limitations under the License.
   }
 }
 
-.icon-button[data-indicator]:is(:active)::before {
+.icon-button:not([aria-disabled="true"])[data-indicator]:is(:active)::before {
   /* Same colour as the background */
   border: var(--cpd-icon-button-indicator-border-size) solid
     var(--cpd-color-bg-subtle-primary);

src/components/IconButton/IconButton.stories.tsx → src/components/Button/IconButton/IconButton.stories.tsx

@@ -25,8 +25,18 @@ export default {
   title: "Button/IconButton",
   component: IconButtonComponent,
   tags: ["autodocs"],
-  argTypes: {},
-  args: {},
+  parameters: {
+    actions: { argTypesRegex: "^on.*" },
+  },
+  argTypes: {
+    control: { type: "boolean" },
+    onClick: { action: "onClick" },
+  },
+  args: {
+    size: "32px",
+    disabled: false,
+    children: <UserIcon />,
+  },
 } as Meta<typeof IconButtonComponent>;
 
 const Template: StoryFn<typeof IconButtonComponent> = (args) => (
@@ -45,15 +55,27 @@ const Template: StoryFn<typeof IconButtonComponent> = (args) => (
   </>
 );
 
-export const Normal = Template.bind({});
-Normal.args = {};
+export const Demo = Template.bind({});
 
-export const WithIndicator = Template.bind({});
-WithIndicator.args = {
-  indicator: "default",
+export const Default = { args: {} };
+export const DefaultDisabled = {
+  args: {
+    disabled: true,
+  },
 };
-
-export const WithHighlightIndicator = Template.bind({});
-WithHighlightIndicator.args = {
-  indicator: "highlight",
+export const WithIndicator = {
+  args: {
+    indicator: "default",
+  },
+};
+export const WithIndicatorDisabled = {
+  args: {
+    indicator: "default",
+    disabled: true,
+  },
+};
+export const WithHighlightIndicator = {
+  args: {
+    indicator: "highlight",
+  },
 };

src/components/Button/IconButton/IconButton.test.tsx

@@ -0,0 +1,53 @@
+/*
+Copyright 2023 New Vector Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { describe, it, expect } from "vitest";
+import { render } from "@testing-library/react";
+import React from "react";
+import { composeStories } from "@storybook/react";
+
+import * as stories from "./IconButton.stories";
+
+const {
+  Default,
+  DefaultDisabled,
+  WithIndicator,
+  WithHighlightIndicator,
+  WithIndicatorDisabled,
+} = composeStories(stories);
+
+describe("IconButton", () => {
+  it("renders a Default IconButton", () => {
+    const { container } = render(<Default />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a DefaultDisabled IconButton", () => {
+    const { container } = render(<DefaultDisabled />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a WithIndicator IconButton", () => {
+    const { container } = render(<WithIndicator />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a WithHighlightIndicator IconButton", () => {
+    const { container } = render(<WithHighlightIndicator />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a WithIndicatorDisabled IconButton", () => {
+    const { container } = render(<WithIndicatorDisabled />);
+    expect(container).toMatchSnapshot();
+  });
+});

src/components/IconButton/IconButton.tsx → src/components/Button/IconButton/IconButton.tsx

@@ -18,22 +18,25 @@ import React, { PropsWithChildren, forwardRef } from "react";
 import classnames from "classnames";
 
 import styles from "./IconButton.module.css";
+import { UnstyledButton } from "../UnstyledButton";
+import { UnstyledButtonPropsFor } from "../UnstyledButton";
 
-type IconButtonProps = JSX.IntrinsicElements["button"] & {
-  /**
-   * The CSS class name.
-   */
-  className?: string;
-  /**
-   * The avatar size in CSS units, e.g. `"24px"`.
-   * @default 32px
-   */
-  size?: CSSStyleDeclaration["height"];
-  /**
-   * The icon button indicator displayed on the top right
-   */
-  indicator?: "default" | "highlight";
-};
+type IconButtonProps = UnstyledButtonPropsFor<"button"> &
+  JSX.IntrinsicElements["button"] & {
+    /**
+     * The CSS class name.
+     */
+    className?: string;
+    /**
+     * The avatar size in CSS units, e.g. `"24px"`.
+     * @default 32px
+     */
+    size?: CSSStyleDeclaration["height"];
+    /**
+     * The icon button indicator displayed on the top right
+     */
+    indicator?: "default" | "highlight";
+  };
 
 /**
  * Display an icon as a button. Can render an indicator
@@ -47,7 +50,8 @@ export const IconButton = forwardRef<
 ) {
   const classes = classnames(styles["icon-button"], className);
   return (
-    <button
+    <UnstyledButton
+      as="button"
       ref={ref}
       className={classes}
       style={
@@ -60,6 +64,6 @@ export const IconButton = forwardRef<
       data-indicator={indicator}
     >
       {React.Children.only(children)}
-    </button>
+    </UnstyledButton>
   );
 });