Skip to content

Feature: Allow passing all Button Props through ClipboardButton component #399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Feature: Allow passing all Button Props through ClipboardButton component #399

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions components/clipboard-button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,25 @@
import { useCopyToClipboard } from '@wordpress/compose';
import { useCopyToClipboard, useMergeRefs } from '@wordpress/compose';
import { useState, useEffect } from '@wordpress/element';
import { Button } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

interface ClipboardButtonProps {
type ButtonProps = React.ComponentProps<typeof Button>;

interface ClipboardButtonProps extends Partial<Omit<ButtonProps, 'children'>> {
/**
* The text to copy to the clipboard.
*/
text: string;

/**
* Boolean to disable the button.
*/
disabled: boolean;

/**
* Function to run when the text is successfully copied.
*/
onSuccess: Function;
onSuccess?: Function;
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a specific function signature instead of the generic Function type. Consider defining it as onSuccess?: () => void for better type safety.

Suggested change
onSuccess?: Function;
onSuccess?: () => void;

Copilot uses AI. Check for mistakes.

/**
* Labels for the button.
*/
labels: ButtonLabels;
labels?: ButtonLabels;
}

interface ButtonLabels {
Expand All @@ -39,9 +36,9 @@ interface ButtonLabels {

export const ClipboardButton: React.FC<ClipboardButtonProps> = ({
text = '',
disabled = false,
onSuccess = () => {},
labels = {},
...rest
}) => {
const [hasCopied, setHasCopied] = useState(false);
const copy = labels.copy ? labels.copy : __('Copy');
Expand Down Expand Up @@ -77,9 +74,10 @@ export const ClipboardButton: React.FC<ClipboardButtonProps> = ({
}

const ref = useCopyToClipboard(text, successfullyCopied);
const mergedRefs = useMergeRefs([ref, (rest.ref as any) || null]);
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ref property should be extracted from rest before spreading to avoid passing it twice to the Button component. Destructure ref from rest at the component parameter level alongside other props.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using as any type assertion. Instead, properly type the ref or use type guards to ensure type safety.

Suggested change
const mergedRefs = useMergeRefs([ref, (rest.ref as any) || null]);
const validRef =
typeof rest.ref === 'function' ||
(rest.ref && typeof rest.ref === 'object' && 'current' in rest.ref)
? rest.ref
: null;
const mergedRefs = useMergeRefs([ref, validRef]);

Copilot uses AI. Check for mistakes.

return (
<Button disabled={disabled} ref={ref}>
<Button {...(rest as any)} ref={mergedRefs}>
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The as any type assertion bypasses type checking. Since rest should already be typed as Partial<Omit<ButtonProps, 'children'>>, the assertion is unnecessary and should be removed.

Suggested change
<Button {...(rest as any)} ref={mergedRefs}>
<Button {...rest} ref={mergedRefs}>

Copilot uses AI. Check for mistakes.
{hasCopied ? copied : copy}
</Button>
);
Expand Down
Loading