chore: address first batch of code review

Author: matoous

packages/circuit-ui/components/CopyButton/CopyButton.spec.tsx

@@ -15,7 +15,14 @@
 
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
-import { act, axe, fireEvent, render, screen } from '../../util/test-utils.js';
+import {
+  act,
+  axe,
+  fireEvent,
+  render,
+  screen,
+  userEvent,
+} from '../../util/test-utils.js';
 import { ToastProvider } from '../ToastContext/index.js';
 
 import { CopyButton } from './CopyButton.js';
@@ -24,7 +31,7 @@ const defaultProps = {
   label: 'API token',
   value: 'secret-token',
   copyLabel: 'Copy token',
-  onCopyLabel: 'Copied to clipboard.',
+  successLabel: 'Copied to clipboard.',
 };
 
 const renderWithToastProvider = (ui: React.ReactNode) =>
@@ -53,7 +60,7 @@ describe('CopyButton', () => {
   });
 
   it('should render text instead of value in the input variant when provided', () => {
-    render(<CopyButton {...defaultProps} text="••••••••••••token" />);
+    render(<CopyButton {...defaultProps} visibleValue="••••••••••••token" />);
 
     expect(screen.getByDisplayValue('••••••••••••token')).toBeVisible();
     expect(
@@ -67,7 +74,7 @@ describe('CopyButton', () => {
     renderWithToastProvider(
       <CopyButton
         {...defaultProps}
-        onCopyLabel="Token copied"
+        successLabel="Token copied"
         onCopy={onCopy}
       />,
     );
@@ -90,7 +97,7 @@ describe('CopyButton', () => {
         copyVariant="button"
         value="secret-token"
         copyLabel="Copy token"
-        onCopyLabel="Copied to clipboard."
+        successLabel="Copied to clipboard."
       />,
     );
 
@@ -108,54 +115,16 @@ describe('CopyButton', () => {
   it('should render the icon variant', () => {
     render(
       <CopyButton
-        copyVariant="icon"
+        copyVariant="icon-button"
         value="secret-token"
         copyLabel="Copy token"
-        onCopyLabel="Copied to clipboard."
+        successLabel="Copied to clipboard."
       />,
     );
 
     expect(screen.getByRole('button', { name: 'Copy token' })).toBeVisible();
   });
 
-  it('should fall back to execCommand when navigator.clipboard is unavailable', async () => {
-    const execCommand = vi.fn();
-    const selection = {
-      addRange: vi.fn(),
-      removeAllRanges: vi.fn(),
-    };
-
-    Object.defineProperty(navigator, 'clipboard', {
-      configurable: true,
-      value: undefined,
-    });
-    Object.defineProperty(document, 'execCommand', {
-      configurable: true,
-      value: execCommand,
-    });
-    Object.defineProperty(window, 'getSelection', {
-      configurable: true,
-      value: vi.fn(() => selection),
-    });
-
-    renderWithToastProvider(
-      <CopyButton {...defaultProps} onCopyLabel="Token copied" />,
-    );
-
-    fireEvent.click(
-      screen.getByRole('button', { name: 'Copy token: API token' }),
-    );
-
-    await act(async () => {
-      await Promise.resolve();
-    });
-
-    expect(execCommand).toHaveBeenCalledWith('copy');
-    expect(selection.removeAllRanges).toHaveBeenCalledTimes(2);
-    expect(selection.addRange).toHaveBeenCalledTimes(1);
-    expect(await screen.findByText('Token copied')).toBeInTheDocument();
-  });
-
   it('should not announce success when clipboard write fails', async () => {
     const onCopy = vi.fn();
 
@@ -181,20 +150,46 @@ describe('CopyButton', () => {
   });
 
   it('should disable copying when the value is empty', () => {
-    render(<CopyButton {...defaultProps} value="" text="N/A" />);
+    render(<CopyButton {...defaultProps} value="" visibleValue="N/A" />);
 
     expect(
       screen.getByRole('button', { name: 'Copy token: API token' }),
     ).toHaveAttribute('aria-disabled', 'true');
   });
 
-  it('should have no accessibility violations', async () => {
-    const { container } = renderWithToastProvider(
-      <CopyButton {...defaultProps} />,
-    );
+  describe('as input', async () => {
+    it('should have no accessibility violations', async () => {
+      const { container } = renderWithToastProvider(
+        <CopyButton {...defaultProps} />,
+      );
+
+      const actual = await axe(container);
 
-    const actual = await axe(container);
+      expect(actual).toHaveNoViolations();
+    });
+  });
 
-    expect(actual).toHaveNoViolations();
+  describe('as button', async () => {
+    it('should have no accessibility violations', async () => {
+      const { container } = renderWithToastProvider(
+        <CopyButton {...defaultProps} copyVariant="button" />,
+      );
+
+      const actual = await axe(container);
+
+      expect(actual).toHaveNoViolations();
+    });
+  });
+
+  describe('as icon button', async () => {
+    it('should have no accessibility violations', async () => {
+      const { container } = renderWithToastProvider(
+        <CopyButton {...defaultProps} copyVariant="icon-button" />,
+      );
+
+      const actual = await axe(container);
+
+      expect(actual).toHaveNoViolations();
+    });
   });
 });

packages/circuit-ui/components/CopyButton/CopyButton.stories.tsx

@@ -30,10 +30,10 @@ export default {
     ),
   ],
   argTypes: {
-    onCopyLabel: { control: 'text' },
+    successLabel: { control: 'text' },
     copyLabel: { control: 'text' },
     copyVariant: {
-      options: ['input', 'button', 'icon'],
+      options: ['input', 'button', 'icon-button'],
       control: { type: 'select' },
     },
     disabled: { control: 'boolean' },
@@ -54,7 +54,7 @@ const inputArgs = {
   label: 'API token',
   value: 'sk_live_1234567890',
   copyLabel: 'Copy token',
-  onCopyLabel: 'Token copied',
+  successLabel: 'Token copied',
 } satisfies CopyButtonProps;
 
 export const Input = (args: CopyButtonProps) => <CopyButton {...args} />;
@@ -74,7 +74,7 @@ LongValue.args = {
   label: 'Webhook secret',
   value: 'whsec_4VbX8i2LwY7nQp3Rk5Tm9Uc1Fd6Hs0Za',
   copyLabel: 'Copy secret',
-  onCopyLabel: 'Secret copied',
+  successLabel: 'Secret copied',
 } satisfies CopyButtonProps;
 
 export const FullButton = (args: CopyButtonProps) => <CopyButton {...args} />;
@@ -83,16 +83,16 @@ FullButton.args = {
   copyVariant: 'button',
   value: 'ref_1234567890',
   copyLabel: 'Copy reference',
-  onCopyLabel: 'Reference copied',
+  successLabel: 'Reference copied',
 } satisfies CopyButtonProps;
 
 export const IconOnly = (args: CopyButtonProps) => <CopyButton {...args} />;
 
 IconOnly.args = {
-  copyVariant: 'icon',
+  copyVariant: 'icon-button',
   value: 'ref_1234567890',
   copyLabel: 'Copy reference',
-  onCopyLabel: 'Reference copied',
+  successLabel: 'Reference copied',
 } satisfies CopyButtonProps;
 
 export const Disabled = (args: CopyButtonProps) => <CopyButton {...args} />;
@@ -128,13 +128,13 @@ export const AllVariants = () => (
       copyVariant="button"
       value="ref_1234567890"
       copyLabel="Copy reference"
-      onCopyLabel="Reference copied"
+      successLabel="Reference copied"
     />
     <CopyButton
-      copyVariant="icon"
+      copyVariant="icon-button"
       value="ref_1234567890"
       copyLabel="Copy reference"
-      onCopyLabel="Reference copied"
+      successLabel="Reference copied"
     />
   </Stack>
 );

packages/circuit-ui/components/CopyButton/CopyButton.tsx

@@ -16,7 +16,6 @@
 'use client';
 
 import { CopyPaste } from '@sumup-oss/icons';
-import { useId } from 'react';
 
 import { Button, IconButton } from '../Button/index.js';
 import { Input, type InputProps } from '../Input/index.js';
@@ -40,7 +39,7 @@ type CommonCopyButtonProps = {
   /**
    * Test copy shown in as a notification after a successful copy action.
    */
-  onCopyLabel: string;
+  successLabel: string;
 };
 
 type InputCopyButtonProps = CommonCopyButtonProps &
@@ -57,7 +56,7 @@ type InputCopyButtonProps = CommonCopyButtonProps &
     /**
      * Optional text rendered inside the field instead of the copied value.
      */
-    text?: string;
+    visibleValue?: string;
   };
 
 type ButtonCopyButtonProps = CommonCopyButtonProps &
@@ -73,62 +72,29 @@ type IconCopyButtonProps = CommonCopyButtonProps &
     /**
      * The CopyButton variant.
      */
-    copyVariant: 'icon';
+    copyVariant: 'icon-button';
   };
 
 export type CopyButtonProps =
   | InputCopyButtonProps
   | ButtonCopyButtonProps
   | IconCopyButtonProps;
 
-async function copyToClipboard(text: string): Promise<void> {
-  if (navigator.clipboard?.writeText) {
-    await navigator.clipboard.writeText(text);
-    return;
-  }
-
-  // Fallback behaviour for older browsers,
-  // see: https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand#browser_compatibility
-  if (typeof document === 'undefined') {
-    return;
-  }
-
-  const selection = window.getSelection();
-  const range = document.createRange();
-  const tempNode = document.createElement('span');
-  tempNode.textContent = text;
-  tempNode.style.position = 'fixed';
-  tempNode.style.opacity = '0';
-  document.body.append(tempNode);
-
-  range.selectNodeContents(tempNode);
-  selection?.removeAllRanges();
-  selection?.addRange(range);
-  document.execCommand?.('copy');
-  selection?.removeAllRanges();
-  tempNode.remove();
-}
-
 /**
  * The CopyButton component copies a provided value to the clipboard and
  * can render as a read-only input, a button, or an icon button.
  */
 export function CopyButton(props: CopyButtonProps) {
-  const generatedId = useId();
   const { setToast } = useNotificationToast();
-  const { onCopyLabel, copyLabel, onCopy, value } = props;
+  const { successLabel, copyLabel, onCopy, value } = props;
   const isCopyDisabled = Boolean(props.disabled || value.length === 0);
 
   const handleCopy = async (event: ClickEvent) => {
-    if (isCopyDisabled) {
-      return;
-    }
-
     try {
-      await copyToClipboard(value);
+      // eslint-disable-next-line compat/compat
+      await navigator.clipboard.writeText(value);
       setToast({
-        body: onCopyLabel,
-        iconLabel: '',
+        body: successLabel,
       });
       onCopy?.(event);
     } catch {
@@ -139,7 +105,7 @@ export function CopyButton(props: CopyButtonProps) {
   if (props.copyVariant === 'button') {
     const {
       copyVariant,
-      onCopyLabel: _onCopyLabel,
+      successLabel: _successLabel,
       copyLabel: _copyLabel,
       onCopy: _onCopy,
       value: _value,
@@ -159,10 +125,10 @@ export function CopyButton(props: CopyButtonProps) {
     );
   }
 
-  if (props.copyVariant === 'icon') {
+  if (props.copyVariant === 'icon-button') {
     const {
       copyVariant,
-      onCopyLabel: _onCopyLabel,
+      successLabel: _successLabel,
       copyLabel: _copyLabel,
       onCopy: _onCopy,
       value: _value,
@@ -184,38 +150,33 @@ export function CopyButton(props: CopyButtonProps) {
 
   const {
     copyVariant,
-    onCopyLabel: _onCopyLabel,
+    successLabel: _successLabel,
     copyLabel: _copyLabel,
-    id: customId,
     inputClassName,
     onCopy: _onCopy,
-    text,
+    visibleValue,
     value: _value,
     ...inputProps
   } = props;
-  const fieldId = customId || generatedId;
-  const displayText = text ?? value;
+  const displayText = visibleValue ?? value;
   const buttonLabel = `${copyLabel}: ${props.label}`;
 
   return (
     <Input
       {...inputProps}
-      id={fieldId}
       value={displayText}
       readOnly
       renderSuffix={(renderProps) => (
         <IconButton
           className={renderProps.className}
           type="button"
           size="s"
-          variant="tertiary"
+          variant="secondary"
           disabled={isCopyDisabled}
           onClick={handleCopy}
-          aria-label={buttonLabel}
-          aria-controls={fieldId}
           icon={CopyPaste}
         >
-          {copyLabel}
+          {buttonLabel}
         </IconButton>
       )}
       inputClassName={inputClassName}