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: Button to copy the contents of the Migration tab to the clipboard #104

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krisnaw
Copy link

@krisnaw krisnaw commented Sep 27, 2024

What kind of change does this PR introduce?

This PR introduces a new Button component that allows users to easily copy the contents of the Migration tab to their clipboard.

What is the current behavior?

Currently, users cannot directly copy the Migration tab's contents.
From this feature request #57

What is the new behavior?

When the button is clicked, its icon will change to a Check icon, indicating that the copy operation has been initiated. Once the copy is complete, the icon will revert to the original Copy icon.

Additional context

367562866-f08b5bea-2d9e-4de8-999f-ff714bdfa1f4

Copy link

vercel bot commented Sep 27, 2024

@krisnaw is attempting to deploy a commit to the Supabase Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@gregnr gregnr left a comment

Choose a reason for hiding this comment

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

Great work @krisnaw! Just a few small comments.

Comment on lines 6 to 8
interface ButtonCopyCodeProps {
code?: string;
}
Copy link
Collaborator

@gregnr gregnr Sep 27, 2024

Choose a reason for hiding this comment

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

A few things:

  1. Let's make this a type (vs interface) just to be consistent with the other components (and export it)
  2. Any good reason to make code optional? Probably a clearer contract to make this required, and then upstream components can conditionally render the copy button if the value is optional.
  3. Maybe we just call this CopyButton with a value prop so that it is more generic and can be used outside code block if we want in the future

<Button
variant="outline"
size="icon"
className="absolute top-4 right-4 z-10"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move layout logic upstream so that this button can be reused more generally.

@krisnaw
Copy link
Author

krisnaw commented Sep 28, 2024

Hi @gregnr, thanks for your feedback!

Here are the updates I've made:

  • Component Name: Changed the component name to CopyButton.
  • Export Type: Changed the export from an interface to an export type for clarity.
  • Required value Prop: Made the value prop required to ensure a clear contract.
  • Layout Upstreaming: Moved the layout logic upstream for better maintainability.
    • Added Tooltip: Included a tooltip for improved user experience.

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.

2 participants