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: Copy account address to clipboard #91

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

Victor-Salomon
Copy link
Collaborator

@Victor-Salomon Victor-Salomon commented Aug 13, 2024

Create an Account component that includes both the Icon & address and implement it in the ongoing and completed transfer avoiding code duplication.

This component also includes the address copy to the clipboard.

UI includes the well know 'Copy icon' that turns green on copy with a notification from our notification system.

Feel free to let me know if you do not like anything.

Capture d’écran 2024-08-13 à 09 50 00

Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
turtle-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 2:38pm
turtle-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 2:38pm

@Victor-Salomon Victor-Salomon requested review from NunoAlexandre and noahjoeris and removed request for NunoAlexandre August 13, 2024 07:22
Copy link
Collaborator

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

This is a sweet nice to have :) nice, @Victor-Salomon 👌

just one comment below

className="flex cursor-pointer items-center space-x-2 text-sm"
>
<p>{content}</p>
<Copy className="h-3 w-3" />
{isCopyIndicator ? (
<CopyCheck className="h-3 w-3 text-turtle-primary" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check with @brandon about these colors? I don't feel that purple should be the main colour for this, it draws too much attention to it 👀 I'd say to use a gray tone for the default and then purple of green for when it's active.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, let me do this.
When Noah validates the PR I will merge it and update if Brandon requires some changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks way better now

Capture d’écran 2024-08-13 à 14 51 44 Capture d’écran 2024-08-13 à 14 51 31

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :)

Copy link
Collaborator

@noahjoeris noahjoeris left a comment

Choose a reason for hiding this comment

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

Nice refactoring!! Thanks Victor. Left some comments

}: {
network: Network
address: string
transferResult?: TransferResult
Copy link
Collaborator

@noahjoeris noahjoeris Aug 13, 2024

Choose a reason for hiding this comment

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

I do not like the prop transferResult. It has nothing to do with an account. You just need it for some styling right?
Can we replace? Maybe an error prop (boolean).


function CopyAddress({ content, address }: { content: string; address: string }) {
const { addNotification } = useNotification()
const [isCopyIndicator, setIsCopyIndicator] = useState(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

better name suggestion: showCopyIndicator

…ylabs-org/turtle into chore/refactor-account-component
Copy link
Collaborator

@NunoAlexandre NunoAlexandre 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 @Victor-Salomon 🥐 👌

Comment on lines +18 to +19
className?: string
allowCopy?: boolean
Copy link
Collaborator

@noahjoeris noahjoeris Aug 13, 2024

Choose a reason for hiding this comment

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

Suggested change
className?: string
allowCopy?: boolean
allowCopy?: boolean
className?: string

Comment on lines +13 to +14
className,
allowCopy = true,
Copy link
Collaborator

@noahjoeris noahjoeris Aug 13, 2024

Choose a reason for hiding this comment

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

Suggested change
className,
allowCopy = true,
allowCopy = true,
className,

Copy link
Collaborator

@noahjoeris noahjoeris left a comment

Choose a reason for hiding this comment

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

amazing thanks Victor 💯

@Victor-Salomon Victor-Salomon changed the title Chore/refactor account component feat: Copy account address to clipboard Aug 13, 2024
@Victor-Salomon Victor-Salomon merged commit 1cbc87a into main Aug 13, 2024
4 of 5 checks passed
@Victor-Salomon Victor-Salomon deleted the chore/refactor-account-component branch August 13, 2024 14:38
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.

3 participants