Skip to content

Conversation

@fabiankaegy
Copy link
Member

Closes #398


This pull request refactors the ClipboardButton component to improve its flexibility and integration with other components. The main changes focus on making the button props more extensible, handling refs correctly, and making some props optional.

Component API improvements

  • The ClipboardButtonProps interface now extends the base Button props (using Partial<Omit<ButtonProps, 'children'>>), allowing the component to accept any valid button prop except for children. This makes the component more flexible and easier to use in different contexts.
  • The disabled, labels, and onSuccess props are now optional, providing sensible defaults and reducing the need for boilerplate when using the component. [1] [2]

Ref handling improvements

  • The component now merges refs using useMergeRefs, ensuring that both internal and external refs are handled correctly when the button is used in more complex scenarios.

Usage and rendering changes

  • The button now spreads any remaining props (rest) onto the underlying Button component, further improving extensibility and allowing for easier customization.

@fabiankaegy fabiankaegy requested review from benlk and Copilot October 27, 2025 11:46
@fabiankaegy fabiankaegy self-assigned this Oct 27, 2025
@fabiankaegy fabiankaegy added the [Type] Enhancement New feature or request label Oct 27, 2025
@fabiankaegy fabiankaegy changed the title feature: allow assing ll button props to the ClipboardButton Feature: Allow passing all Button Props through ClipboardButton component Oct 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the ClipboardButton component by making it more flexible and allowing consumers to pass additional button props. The component now extends ButtonProps, properly handles external refs, and makes several props optional with sensible defaults.

Key Changes:

  • Extended ClipboardButtonProps to accept all standard button props except children
  • Made disabled, labels, and onSuccess props optional
  • Implemented proper ref merging to handle both internal and external refs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

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.
}

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.

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.
* 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.
@cypress
Copy link

cypress bot commented Oct 27, 2025

10up Block Components    Run #1061

Run Properties:  status check passed Passed #1061  •  git commit c710159613: feature: allow assing ll button props to the ClipboardButton
Project 10up Block Components
Branch Review feature/clipboard-button-rest-props
Run status status check passed Passed #1061
Run duration 04m 55s
Commit git commit c710159613: feature: allow assing ll button props to the ClipboardButton
Committer Fabian Kägy
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 35
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ClipboardButton: Pass through props to underlying Button

1 participant