Skip to content
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.

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

feat!: use native copy method #1852

Open
wants to merge 8 commits into
base: next
Choose a base branch
from
Open

feat!: use native copy method #1852

wants to merge 8 commits into from

Conversation

ogonkov
Copy link
Contributor

@ogonkov ogonkov commented Sep 12, 2024

Breaking changes:

  • native copy method used
  • options of copy-to-clipboard removed from props

Fixes #658

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

src/utils/copyText.ts Outdated Show resolved Hide resolved
src/components/CopyToClipboard/CopyToClipboard.tsx Outdated Show resolved Hide resolved
@ogonkov ogonkov requested a review from Raubzeug October 11, 2024 19:40
src/utils/copyText.ts Outdated Show resolved Hide resolved
@ogonkov ogonkov changed the title feat: add native copy method feat!: add native copy method Dec 25, 2024
@ogonkov ogonkov changed the title feat!: add native copy method feat!: use native copy method Dec 25, 2024
@ogonkov ogonkov requested a review from Raubzeug December 25, 2024 16:26
const content = React.useMemo<React.ReactElement<React.HTMLAttributes<HTMLElement>>>(
() => children(status),
[children, status],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's no benefit of useMemo at all, children as function typically inline in jsx, so every render is new instance

if (text === textRef.current) {
handleCopy(text, result);

content.props?.onClick?.(event);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we call this onClick unconditionally?

/>
</ReactCopyToClipboard>
<CopyToClipboard text={copyText}>
{() => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's support normal children type as well as it was in ReactCopyToClipboard ? I find it usefull, if I don't need status feedback

@@ -0,0 +1,7 @@
export function copyText(text: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not needed to be at the root utils, place it near the component please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of react-copy-to-clipboard
4 participants