Cleanup button props and consolidate IconButton into Button (#1502)

* Remove legacy props

* Pass icon as part of children

* Use onClick inside PanelHeaderActionButton

* Remove unsued props for PanelHeaderActionButton

* Use IconButton instead of PanelHeaderActionButton

* Integrate IconButton functionality in Button

* Fix some icon buttons

* Use Button instead of IconButton

* Update agents

* Consolidate button styles

* Rename variants

* Rename base size to default

* Use tooltip instead of title

* Add JSDocs and tests

* Use aria-label instead of span in children

* Use asChild

* Add NavButton

* Simplify RouteButtons

* Cleanup

Author: kmcginnes

AGENTS.md

@@ -4,6 +4,7 @@
 
 - Keep conversations concise. Sacrifice grammar for brevity.
 - Use TypeScript for type safety
+- Do not change the VS Code setting `typescript.autoClosingTags`
 - Prefer descriptive variable and function names over code comments
 - Prefer simple to follow logic over clever concise code
 - Prefer named function syntax over anonymous arrow functions for module-level

packages/graph-explorer/src/components/Button/Button.test.tsx

@@ -0,0 +1,91 @@
+import type { PropsWithChildren } from "react";
+
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+
+import { TooltipProvider } from "../Tooltip";
+import { Button, stopPropagation } from "./Button";
+
+function Wrapper({ children }: PropsWithChildren) {
+  return <TooltipProvider>{children}</TooltipProvider>;
+}
+
+describe("Button", () => {
+  test("should render children", () => {
+    render(<Button>Click me</Button>);
+    expect(screen.getByRole("button")).toHaveTextContent("Click me");
+  });
+
+  test("should render tooltip when tooltip prop is provided", async () => {
+    const user = userEvent.setup();
+    render(
+      <Wrapper>
+        <Button tooltip="Helpful tip">Hover me</Button>
+      </Wrapper>,
+    );
+
+    const button = screen.getByRole("button");
+    await user.hover(button);
+
+    expect(await screen.findByRole("tooltip")).toHaveTextContent("Helpful tip");
+  });
+
+  test("should set aria-label when tooltip is provided", () => {
+    render(
+      <Wrapper>
+        <Button tooltip="Accessible label">Click me</Button>
+      </Wrapper>,
+    );
+
+    expect(screen.getByRole("button")).toHaveAttribute(
+      "aria-label",
+      "Accessible label",
+    );
+  });
+
+  test("should not render tooltip when tooltip is not provided", () => {
+    render(<Button>No tooltip</Button>);
+    expect(screen.queryByRole("tooltip")).not.toBeInTheDocument();
+  });
+
+  test("should handle onClick events", async () => {
+    const user = userEvent.setup();
+    const handleClick = vi.fn();
+
+    render(<Button onClick={handleClick}>Click me</Button>);
+
+    await user.click(screen.getByRole("button"));
+    expect(handleClick).toHaveBeenCalledTimes(1);
+  });
+
+  test("should be disabled when disabled prop is true", () => {
+    render(<Button disabled>Disabled</Button>);
+    expect(screen.getByRole("button")).toBeDisabled();
+  });
+});
+
+describe("stopPropagation", () => {
+  test("should stop event propagation and call action", async () => {
+    const action = vi.fn();
+    const event = {
+      stopPropagation: vi.fn(),
+    } as unknown as React.MouseEvent<HTMLButtonElement>;
+
+    await stopPropagation(action)(event);
+
+    expect(event.stopPropagation).toHaveBeenCalled();
+    expect(action).toHaveBeenCalled();
+  });
+
+  test("should handle async actions", async () => {
+    const action = vi.fn().mockResolvedValue(undefined);
+    const event = {
+      stopPropagation: vi.fn(),
+    } as unknown as React.MouseEvent<HTMLButtonElement>;
+
+    await stopPropagation(action)(event);
+
+    expect(event.stopPropagation).toHaveBeenCalled();
+    expect(action).toHaveBeenCalled();
+  });
+});

packages/graph-explorer/src/components/Button/Button.tsx

@@ -1,128 +1,96 @@
-import type { ReactNode } from "react";
-
 import { cva, type VariantProps } from "cva";
 import { Slot } from "radix-ui";
 
 import { cn } from "@/utils";
 
-export const buttonStyles = cva({
+import { Tooltip, TooltipContent, TooltipTrigger } from "../Tooltip";
+
+const buttonStyles = cva({
   base: "inline-flex items-center justify-center gap-2 font-medium focus-visible:ring-1 focus-visible:outline-hidden disabled:pointer-events-none disabled:opacity-50 disabled:saturate-0 aria-disabled:pointer-events-none aria-disabled:opacity-50 aria-disabled:saturate-0 [&_svg]:pointer-events-none [&_svg]:shrink-0",
   variants: {
     variant: {
-      filled: "",
-      default: "",
-      text: "",
-      outline: "",
-    },
-    color: {
-      primary: "",
-      danger: "",
-    },
-    size: {
-      small: "h-8 rounded-md px-2 text-sm [&_svg]:size-4",
-      base: "h-10 rounded-md px-4 text-base [&_svg]:size-5",
-      large: "h-12 rounded-md px-5 text-lg [&_svg]:size-6",
-    },
-  },
-  compoundVariants: [
-    {
-      variant: "filled",
-      color: "primary",
-      className:
+      primary:
         "bg-brand hover:bg-brand-hover data-open:bg-brand-hover text-white",
-    },
-    {
-      variant: "filled",
-      color: "danger",
-      className:
-        "bg-danger hover:bg-danger-hover data-open:bg-danger-hover text-white",
-    },
-    {
-      variant: "default",
-      color: "primary",
-      className:
+      secondary:
         "text-text-primary bg-gray-100 hover:bg-gray-200 data-open:bg-gray-200",
-    },
-    {
-      variant: "default",
-      color: "danger",
-      className:
-        "bg-danger-subtle text-danger hover:bg-danger-subtle-hover data-open:bg-danger-subtle-hover",
-    },
-    {
-      variant: "text",
-      color: "primary",
-      className:
+      ghost:
         "text-primary-foreground hover:bg-primary-subtle data-open:bg-primary-subtle",
-    },
-    {
-      variant: "text",
-      color: "danger",
-      className:
-        "text-danger hover:bg-danger-subtle data-open:bg-danger-subtle",
-    },
-    {
-      variant: "outline",
-      color: "primary",
-      className:
+      outline:
         "text-text-primary border-input hover:bg-muted data-open:border-primary-main border bg-transparent shadow-xs",
+      danger:
+        "bg-danger-subtle text-danger hover:bg-danger-subtle-hover data-open:bg-danger-subtle-hover",
+      "danger-ghost":
+        "text-danger hover:bg-danger-subtle data-open:bg-danger-subtle",
     },
-    {
-      variant: "outline",
-      color: "danger",
-      className:
-        "border-error-light text-danger hover:bg-danger-subtle data-open:border-error-main border shadow-xs",
+    size: {
+      small: "h-8 rounded-md px-2 text-sm [&_svg]:size-4",
+      default: "h-10 rounded-md px-4 text-base [&_svg]:size-5",
+      large: "h-12 rounded-md px-5 text-lg [&_svg]:size-6",
+
+      "icon-small": "h-8 min-w-8 rounded-md text-sm [&_svg]:size-4",
+      icon: "h-10 min-w-10 rounded-md text-base [&_svg]:size-[1.325rem]",
+      "icon-large": "h-12 min-w-12 rounded-md text-lg [&_svg]:size-6",
     },
-  ],
+  },
   defaultVariants: {
-    variant: "default",
-    color: "primary",
-    size: "base",
+    variant: "secondary",
+    size: "default",
   },
 });
 
 export interface ButtonProps
   extends
-    VariantProps<typeof buttonStyles>,
     VariantProps<typeof buttonStyles>,
     Omit<React.ComponentPropsWithRef<"button">, "color"> {
-  icon?: ReactNode;
   asChild?: boolean;
-
-  // TODO: Remove these non-standard props and use the `disabled` and `onClick` props instead.
-  // These are here to reduce the amount of changes to the rest of the codebase.
-  isDisabled?: boolean;
-  onPress?: () => void;
+  /** When provided, displays a tooltip and adds screen reader text. */
+  tooltip?: string;
 }
 
+/**
+ * A flexible button component supporting multiple variants and sizes.
+ * When `tooltip` is provided, renders with a tooltip and screen reader text.
+ */
 function Button({
   className,
-  icon,
-  variant = "default",
+  variant = "secondary",
   size,
-  color,
+  tooltip,
   children,
   asChild = false,
-  isDisabled,
-  onPress,
   ...props
 }: ButtonProps) {
   const Component = asChild ? (Slot.Root as any) : "button";
-  return (
+  const content = (
     <Component
-      className={cn(buttonStyles({ size, variant, color }), className)}
-      disabled={isDisabled}
-      onClick={onPress}
+      data-slot="button"
+      data-variant={variant}
+      data-size={size}
+      aria-label={tooltip}
+      className={cn(buttonStyles({ size, variant }), className)}
       {...props}
     >
-      {icon && icon}
       {children}
     </Component>
   );
+
+  if (tooltip) {
+    return (
+      <Tooltip>
+        <TooltipTrigger asChild>{content}</TooltipTrigger>
+        <TooltipContent>{tooltip}</TooltipContent>
+      </Tooltip>
+    );
+  }
+
+  return content;
 }
 Button.displayName = "Button";
 
-/** Wrap an action to stop button click propagation. */
+/**
+ * Wraps an action to stop click event propagation before executing.
+ * Useful for buttons inside clickable containers.
+ */
 export function stopPropagation(action: () => void | Promise<void>) {
   return async (event: React.MouseEvent<HTMLButtonElement>) => {
     event.stopPropagation();

packages/graph-explorer/src/components/Button/NavButton.test.tsx

@@ -0,0 +1,82 @@
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import { MemoryRouter, useLocation } from "react-router";
+
+import { NavButton } from "./NavButton";
+
+function LocationDisplay() {
+  const location = useLocation();
+  return <div data-testid="location">{location.pathname}</div>;
+}
+
+describe("NavButton", () => {
+  test("should render children", () => {
+    render(
+      <MemoryRouter>
+        <NavButton to="/test">Click me</NavButton>
+      </MemoryRouter>,
+    );
+    expect(screen.getByRole("button")).toHaveTextContent("Click me");
+  });
+
+  test("should navigate when clicked", async () => {
+    const user = userEvent.setup();
+
+    render(
+      <MemoryRouter initialEntries={["/"]}>
+        <NavButton to="/destination">Navigate</NavButton>
+        <LocationDisplay />
+      </MemoryRouter>,
+    );
+
+    expect(screen.getByTestId("location")).toHaveTextContent("/");
+
+    await user.click(screen.getByRole("button"));
+
+    expect(screen.getByTestId("location")).toHaveTextContent("/destination");
+  });
+
+  test("should not navigate when disabled", async () => {
+    const user = userEvent.setup();
+
+    render(
+      <MemoryRouter initialEntries={["/"]}>
+        <NavButton to="/destination" disabled>
+          Navigate
+        </NavButton>
+        <LocationDisplay />
+      </MemoryRouter>,
+    );
+
+    expect(screen.getByTestId("location")).toHaveTextContent("/");
+
+    await user.click(screen.getByRole("button"));
+
+    expect(screen.getByTestId("location")).toHaveTextContent("/");
+  });
+
+  test("should be disabled when disabled prop is true", () => {
+    render(
+      <MemoryRouter>
+        <NavButton to="/test" disabled>
+          Disabled
+        </NavButton>
+      </MemoryRouter>,
+    );
+    expect(screen.getByRole("button")).toBeDisabled();
+  });
+
+  test("should pass variant to Button", () => {
+    render(
+      <MemoryRouter>
+        <NavButton to="/test" variant="primary">
+          Primary
+        </NavButton>
+      </MemoryRouter>,
+    );
+    expect(screen.getByRole("button")).toHaveAttribute(
+      "data-variant",
+      "primary",
+    );
+  });
+});

packages/graph-explorer/src/components/Button/NavButton.tsx

@@ -0,0 +1,33 @@
+import type { ComponentPropsWithRef } from "react";
+import type { NavigateOptions, To } from "react-router";
+
+import { useNavigate } from "react-router";
+
+import { Button } from "./Button";
+
+export interface NavButtonProps extends Omit<
+  ComponentPropsWithRef<typeof Button>,
+  "onClick" | "asChild"
+> {
+  /** The path to navigate to when clicked. */
+  to: To;
+  /** Options for the navigation. */
+  navOptions?: NavigateOptions;
+}
+
+/**
+ * A button component that navigates to a route when clicked.
+ * Unlike using Button with asChild and Link, this component prevents
+ * navigation when disabled.
+ */
+function NavButton({ to, navOptions, ...props }: NavButtonProps) {
+  const navigate = useNavigate();
+
+  function handleClick() {
+    navigate(to, navOptions);
+  }
+
+  return <Button {...props} onClick={handleClick} />;
+}
+
+export { NavButton };