fix(Button): type="button" をデフォルトで設定, loading時にdisabledにしないように修正 (#…

…1482) * Fix button to add type="button" as default, and remove disabled when loading * Add test for focusable when loading * Fix type definiton for onClick * Fix test * Add change log
4-design · Jan 4, 2024 · 9a7278e · 9a7278e
1 parent ea8110a
commit 9a7278e
Show file tree

Hide file tree

Showing 3 changed files with 178 additions and 149 deletions.
diff --git a/.changeset/khaki-doors-nail.md b/.changeset/khaki-doors-nail.md
@@ -0,0 +1,5 @@
+---
+"@4design/for-ui": patch
+---
+
+fix(Button): `type="button"` をデフォルトで設定, loading時にdisabledにしないように修正
diff --git a/packages/for-ui/src/button/Button.test.tsx b/packages/for-ui/src/button/Button.test.tsx
@@ -36,15 +36,15 @@ describe('Button', () => {
     );
     expect(screen.queryByRole('button', { name: 'button' })).toBeInTheDocument();
   });
-  it('with nested text renders single label', () => {
+  it('with nested text is rendered', () => {
     render(
       <Button>
         <Text>
-          but<Text>ton</Text>
+          button<Text>test</Text>
         </Text>
       </Button>,
     );
-    expect(screen.queryByRole('button', { name: 'button' })).toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'button test' })).toBeInTheDocument();
   });
   it('does not fire onClick event when not clicked', async () => {
     const onClick = vi.fn();
@@ -77,7 +77,19 @@ describe('Button', () => {
         button
       </Button>,
     );
-    await user.click(screen.getByRole('button', { name: 'button' }));
+    const element = screen.getByText('button')?.closest('button');
+    expect(element).toBeTruthy();
+    if (!element) return; // Workaround for that expect does not assert element is not null
+    await user.click(element);
     expect(onClick).not.toHaveBeenCalled();
   });
+  it('works as link when specified as anchor tag by `as`', async () => {
+    render(
+      <Button as="a" href="https://example.com">
+        test
+      </Button>,
+    );
+    const element = await screen.findByRole('link', { name: 'test' });
+    expect(element).toBeInTheDocument();
+  });
 });
diff --git a/packages/for-ui/src/button/Button.tsx b/packages/for-ui/src/button/Button.tsx
@@ -1,15 +1,16 @@
-import { Children, ComponentPropsWithoutRef, ElementType, forwardRef, ReactNode, Ref, useMemo } from 'react';
+import { Children, ElementType, FC, forwardRef, MouseEvent, MouseEventHandler, ReactNode, useMemo } from 'react';
 import MuiButton, { ButtonUnstyledProps as MuiButtonProps } from '@mui/base/ButtonUnstyled';
 import { LoadingButtonProps } from '@mui/lab/LoadingButton';
 import { Loader } from '../loader';
 import { fsx } from '../system/fsx';
+import { ComponentProps, Ref } from '../system/polyComponent';
 import { walkChildren } from '../system/walkChildren';
 
 // Iterable<ReactNode> seems to contain string but cannot be excluded, so added as sum type.
 type Child = Exclude<ReactNode, Iterable<ReactNode>> | string;
 
-export type ButtonProps<As extends ElementType = 'button'> = MuiButtonProps<As> &
-  ComponentPropsWithoutRef<As> & {
+export type ButtonProps<As extends ElementType = 'button'> = ComponentProps<
+  Omit<MuiButtonProps<As>, 'href' | 'children' | 'onClick'> & {
     /**
      * 種類を指定
      *
@@ -96,8 +97,12 @@ export type ButtonProps<As extends ElementType = 'button'> = MuiButtonProps<As>
      */
     color?: 'primary' | 'secondary' | 'default';
 
+    onClick?: MouseEventHandler<HTMLElementTagNameMap[As extends keyof HTMLElementTagNameMap ? As : 'button']>;
+
     className?: string;
-  };
+  },
+  As
+>;
 
 const extractText = (root: ReactNode): string => {
   let ret = '';
@@ -118,148 +123,155 @@ const structures = ['text', 'icon', 'text-icon', 'icon-text'] as const;
 
 type Structure = (typeof structures)[number];
 
-const _Button = <As extends ElementType = 'button'>({
-  as,
-  variant = 'outlined',
-  intention: passedIntention = 'subtle',
-  size = 'large',
-  disabled = false,
-  loading = false,
-  startIcon,
-  endIcon,
-  color,
-  children,
-  _ref,
-  className,
-  ...rest
-}: ButtonProps<As> & { _ref?: Ref<As> }): JSX.Element => {
-  const component = as || 'button';
-  const childTexts = useMemo(() => Children.map(children, extractText) || [], [children]);
-  const label = childTexts.join('');
-  const structure: Structure = useMemo(() => {
-    if ((childTexts.at(0) && !childTexts.at(-1)) || (endIcon && children)) {
-      return 'text-icon';
-    }
-    if ((!childTexts.at(0) && childTexts.at(-1)) || (startIcon && children)) {
-      return 'icon-text';
-    }
-    if (!childTexts.at(0) || (startIcon && !children)) {
-      return 'icon';
-    }
-    return 'text';
-  }, [startIcon, endIcon, children, childTexts]);
+type ButtonComponent = <As extends ElementType = 'button'>(props: ButtonProps<As>) => ReturnType<FC>;
 
-  // Legacy support for color props
-  // If not needed, rename the passedIntention to intention.
+export const Button: ButtonComponent = forwardRef(
+  <As extends ElementType = 'button'>(
+    {
+      as,
+      variant = 'outlined',
+      intention: passedIntention = 'subtle',
+      size = 'large',
+      loading = false,
+      startIcon,
+      endIcon,
+      color,
+      children,
+      className,
+      onClick,
+      ...rest
+    }: ButtonProps<As>,
+    ref?: Ref<As>,
+  ): JSX.Element => {
+    const component = as || 'button';
+    const childTexts = useMemo(() => Children.map(children, extractText) || [], [children]);
+    const structure: Structure = useMemo(() => {
+      if ((childTexts.at(0) && !childTexts.at(-1)) || (endIcon && children)) {
+        return 'text-icon';
+      }
+      if ((!childTexts.at(0) && childTexts.at(-1)) || (startIcon && children)) {
+        return 'icon-text';
+      }
+      if (!childTexts.at(0) || (startIcon && !children)) {
+        return 'icon';
+      }
+      return 'text';
+    }, [startIcon, endIcon, children, childTexts]);
 
-  const intention = color
-    ? (
-        {
-          primary: 'primary',
-          secondary: 'secondary',
-          default: 'primary',
-        } as const
-      )[color]
-    : passedIntention;
+    // Legacy support for color props
+    // If not needed, rename the passedIntention to intention.
 
-  return (
-    <MuiButton<As>
-      component={component}
-      ref={_ref}
-      disabled={disabled || loading}
-      aria-label={label || rest['aria-label'] || 'button'}
-      aria-busy={loading}
-      className={fsx([
-        `rounded-1.5 focus-visible:shadow-focused relative flex h-fit w-fit shrink-0 flex-row items-center justify-center font-sans outline-none disabled:cursor-not-allowed [&_svg]:fill-inherit`,
-        {
-          text: {
-            large: `px-4 py-2 gap-1`,
-            medium: `px-4 py-1 gap-1`,
-            small: `px-2 py-0.5 gap-0.5`,
-          },
-          icon: {
-            large: `p-2 gap-1 [&_svg]:w-6 [&_svg]:h-6`,
-            medium: `p-1 gap-1 [&_svg]:w-6 [&_svg]:h-6`,
-            small: `p-1 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
-          },
-          'text-icon': {
-            large: `pl-4 pr-3 py-2 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
-            medium: `pl-4 pr-2 py-1 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
-            small: `pl-2 pr-1 py-0.5 gap-0.5 [&_svg]:w-3 [&_svg]:h-3`,
-          },
-          'icon-text': {
-            large: `pl-3 pr-4 py-2 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
-            medium: `pl-2 pr-4 py-1 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
-            small: `pl-1 pr-2 py-0.5 gap-0.5 [&_svg]:w-3 [&_svg]:h-3`,
-          },
-        }[structure][size],
-        {
-          large: `text-r`,
-          medium: `text-r`,
-          small: `text-s`,
-        }[size],
-        {
-          filled: `font-bold disabled:bg-shade-dark-disabled disabled:text-shade-white-disabled disabled:fill-shade-dark-disabled`,
-          outlined: `font-regular outline outline-1 -outline-offset-1 disabled:bg-shade-dark-disabled disabled:outline-shade-dark-disabled disabled:text-shade-white-disabled disabled:fill-shade-dark-disabled`,
-          text: `font-bold disabled:bg-shade-dark-disabled disabled:text-shade-white-disabled disabled:fill-shade-dark-disabled`,
-        }[variant],
-        {
-          subtle: {
-            filled: [
-              `bg-shade-light-default hover:bg-shade-light-hover focus-visible:bg-shade-light-hover text-shade-medium-default fill-shade-medium-default`,
-              structure === 'icon' && `fill-shade-dark-default`,
-            ],
-            outlined: [
-              `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover outline-shade-medium-default text-shade-dark-default fill-shade-medium-default`,
-              structure === 'icon' && `fill-shade-dark-default`,
-            ],
-            text: [
-              `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover text-shade-medium-default fill-shade-medium-default`,
-              structure === 'icon' && `fill-shade-dark-default`,
-            ],
-          },
-          primary: {
-            filled: `bg-primary-dark-default hover:bg-primary-dark-hover focus-visible:bg-primary-dark-hover text-shade-white-default fill-shade-white-default`,
-            outlined: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover outline-primary-dark-default text-primary-dark-default fill-primary-dark-default`,
-            text: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover text-primary-dark-default fill-primary-dark-default`,
-          },
-          secondary: {
-            filled: `bg-secondary-dark-default hover:bg-secondary-dark-hover focus-visible:bg-secondary-dark-hover text-shade-white-default fill-shade-white-default`,
-            outlined: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover outline-secondary-dark-default text-secondary-dark-default fill-secondary-dark-default`,
-            text: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover text-secondary-dark-default fill-secondary-dark-default`,
-          },
-          shade: {
-            filled: `bg-shade-dark-default hover:bg-shade-dark-hover text-shade-white-default fill-shade-white-default`,
-            outlined: `bg-shade-white-default hover:bg-shade-white-hover outline-shade-dark-default text-shade-dark-default fill-shade-dark-default`,
-            text: `bg-shade-white-default hover:bg-shade-white-hover text-shade-dark-default fill-shade-dark-default`,
-          },
-          negative: {
-            filled: `bg-negative-dark-default hover:bg-negative-dark-hover text-shade-white-default fill-shade-white-default`,
-            outlined: `bg-shade-white-default hover:bg-shade-white-hover outline-negative-dark-default text-negative-dark-default fill-negative-dark-default`,
-            text: `bg-shade-white-default hover:bg-shade-white-hover text-negative-dark-default fill-negative-dark-default`,
-          },
-        }[intention][variant],
-        className,
-      ])}
-      // FIXME: Avoid unintended type error, maybe MUI's problem?
-      {...(rest as MuiButtonProps<As>)}
-    >
-      {startIcon}
-      {children}
-      {endIcon}
-      {loading && (
-        <div className={fsx(`absolute inset-0 grid h-full w-full place-items-center`)}>
-          <Loader
-            className={fsx(
-              `[&:is(svg):is(svg)]:fill-shade-dark-default [&:is(svg):is(svg)]:h-6 [&:is(svg):is(svg)]:w-6`,
-            )}
-          />
-        </div>
-      )}
-    </MuiButton>
-  );
-};
+    const intention = color
+      ? (
+          {
+            primary: 'primary',
+            secondary: 'secondary',
+            default: 'primary',
+          } as const
+        )[color]
+      : passedIntention;
 
-export const Button = forwardRef((props: ButtonProps<ElementType>, ref: Ref<ElementType>) => (
-  <_Button _ref={ref} {...props} />
-)) as <As extends ElementType = 'button'>(props: ButtonProps<As>) => JSX.Element;
+    return (
+      <MuiButton<As>
+        component={component}
+        ref={ref}
+        aria-disabled={loading}
+        aria-busy={loading}
+        type="button"
+        className={fsx([
+          `rounded-1.5 focus-visible:shadow-focused relative flex h-fit w-fit shrink-0 flex-row items-center justify-center font-sans outline-none disabled:cursor-not-allowed [&_svg]:fill-inherit`,
+          {
+            text: {
+              large: `px-4 py-2 gap-1`,
+              medium: `px-4 py-1 gap-1`,
+              small: `px-2 py-0.5 gap-0.5`,
+            },
+            icon: {
+              large: `p-2 gap-1 [&_svg]:w-6 [&_svg]:h-6`,
+              medium: `p-1 gap-1 [&_svg]:w-6 [&_svg]:h-6`,
+              small: `p-1 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
+            },
+            'text-icon': {
+              large: `pl-4 pr-3 py-2 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
+              medium: `pl-4 pr-2 py-1 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
+              small: `pl-2 pr-1 py-0.5 gap-0.5 [&_svg]:w-3 [&_svg]:h-3`,
+            },
+            'icon-text': {
+              large: `pl-3 pr-4 py-2 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
+              medium: `pl-2 pr-4 py-1 gap-1 [&_svg]:w-4 [&_svg]:h-4`,
+              small: `pl-1 pr-2 py-0.5 gap-0.5 [&_svg]:w-3 [&_svg]:h-3`,
+            },
+          }[structure][size],
+          {
+            large: `text-r`,
+            medium: `text-r`,
+            small: `text-s`,
+          }[size],
+          {
+            filled: `font-bold disabled:bg-shade-dark-disabled disabled:text-shade-white-disabled disabled:fill-shade-dark-disabled`,
+            outlined: `font-regular outline outline-1 -outline-offset-1 disabled:bg-shade-dark-disabled disabled:outline-shade-dark-disabled disabled:text-shade-white-disabled disabled:fill-shade-dark-disabled`,
+            text: `font-bold disabled:bg-shade-dark-disabled disabled:text-shade-white-disabled disabled:fill-shade-dark-disabled`,
+          }[variant],
+          {
+            subtle: {
+              filled: [
+                `bg-shade-light-default hover:bg-shade-light-hover focus-visible:bg-shade-light-hover text-shade-medium-default fill-shade-medium-default`,
+                structure === 'icon' && `fill-shade-dark-default`,
+              ],
+              outlined: [
+                `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover outline-shade-medium-default text-shade-dark-default fill-shade-medium-default`,
+                structure === 'icon' && `fill-shade-dark-default`,
+              ],
+              text: [
+                `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover text-shade-medium-default fill-shade-medium-default`,
+                structure === 'icon' && `fill-shade-dark-default`,
+              ],
+            },
+            primary: {
+              filled: `bg-primary-dark-default hover:bg-primary-dark-hover focus-visible:bg-primary-dark-hover text-shade-white-default fill-shade-white-default`,
+              outlined: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover outline-primary-dark-default text-primary-dark-default fill-primary-dark-default`,
+              text: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover text-primary-dark-default fill-primary-dark-default`,
+            },
+            secondary: {
+              filled: `bg-secondary-dark-default hover:bg-secondary-dark-hover focus-visible:bg-secondary-dark-hover text-shade-white-default fill-shade-white-default`,
+              outlined: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover outline-secondary-dark-default text-secondary-dark-default fill-secondary-dark-default`,
+              text: `bg-shade-white-default hover:bg-shade-white-hover focus-visible:bg-shade-white-hover text-secondary-dark-default fill-secondary-dark-default`,
+            },
+            shade: {
+              filled: `bg-shade-dark-default hover:bg-shade-dark-hover text-shade-white-default fill-shade-white-default`,
+              outlined: `bg-shade-white-default hover:bg-shade-white-hover outline-shade-dark-default text-shade-dark-default fill-shade-dark-default`,
+              text: `bg-shade-white-default hover:bg-shade-white-hover text-shade-dark-default fill-shade-dark-default`,
+            },
+            negative: {
+              filled: `bg-negative-dark-default hover:bg-negative-dark-hover text-shade-white-default fill-shade-white-default`,
+              outlined: `bg-shade-white-default hover:bg-shade-white-hover outline-negative-dark-default text-negative-dark-default fill-negative-dark-default`,
+              text: `bg-shade-white-default hover:bg-shade-white-hover text-negative-dark-default fill-negative-dark-default`,
+            },
+          }[intention][variant],
+          className,
+        ])}
+        // FIXME: Avoid unintended type error, maybe MUI's problem?
+        {...(rest as MuiButtonProps<As>)}
+        onClick={(e: MouseEvent<HTMLElementTagNameMap[As extends keyof HTMLElementTagNameMap ? As : 'button']>) => {
+          if (loading) {
+            return;
+          }
+          onClick?.(e);
+        }}
+      >
+        {startIcon}
+        {children}
+        {endIcon}
+        {loading && (
+          <div className={fsx(`absolute inset-0 grid h-full w-full place-items-center`)}>
+            <Loader
+              className={fsx(
+                `[&:is(svg):is(svg)]:fill-shade-dark-default [&:is(svg):is(svg)]:h-6 [&:is(svg):is(svg)]:w-6`,
+              )}
+            />
+          </div>
+        )}
+      </MuiButton>
+    );
+  },
+);