-
Notifications
You must be signed in to change notification settings - Fork 153
Feat: address can be copied to clipboard #105
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
Conversation
… to use a couple of icons
…iable boolean prop - make sure we copy input value to clipboard when clicking on input - display icons at far right of the input when copiable is set to true as well as a feedback check mark when copied
…t with copiable set to true - add tests to make sure input is copiable
🦋 Changeset detectedLatest commit: 9ab9621 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
No ready to review - will resolve conflicts tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I also noticed that the cursor doesn't change to type hover
when I hover over the copy button. Can you fix that please?
Object.assign(navigator, { | ||
clipboard: { | ||
writeText: () => {}, | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here that explains what this code does? (More importantly, why it was added?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in c12b761
const handleClick = async (event: SyntheticEvent): Promise<void> => { | ||
const value = event.currentTarget.value as string; | ||
|
||
setError(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we setting error
to null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed as part of dcd5eb6
const value = event.currentTarget.value as string; | ||
|
||
setError(null); | ||
setCopied(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary since it copied
will be true
almost instantly anyways later on in this very function.
What I suggest is that we set a timeout or something that sets copied
back to false
after 2 or 3 seconds so that the user can see the 'copy' icon after 2-3 seconds if they want to copy it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good idea - fixed in dcd5eb6
|
||
export interface AddressProps { | ||
/** | ||
* The address to display | ||
*/ | ||
value: string; | ||
/** | ||
* Wheter the address can be copied or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here - should be Whether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed as part of dcd5eb6
… going back to default state
@@ -32,5 +54,40 @@ export const Address: FC<AddressProps> = ({ value, shortened = false }) => { | |||
displayAddress = value; | |||
} | |||
|
|||
return <Input value={displayAddress} />; | |||
const handleClick = async (event: SyntheticEvent): Promise<void> => { | |||
const value = event.currentTarget.value as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should copy the props.value
instead of the input.value
as it could be shortened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in f16b638
… be shortened and it makes more sense to copy the FULL address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @maximebonhomme! Can you just fix the cursor so that it is a 'pointer' when hovering over the copy button? Thanks!
@Dhaiwat10 added! Will create a ticket to convert that input into a text component as mentionned in Discord. |
Let's wait for PR#95 to be merged first and I'll fix any conflicts
Closes #86
Description
This PR includes changes to make the Address component copiable to clipboard. We're now able to provide a prop
<Address value='0x000' copiable />
to enable users to copy the address to their clipboard.What changed
Address
calledcopiable
as typeboolean
- default is falsycopiable
is set totrue
we're now displaying a Copy Icon on the far right of the input - This allows users to know they can copy the address to their clipboard📝 Additional Information
I have added
@chakra-ui/icons
package to add icons, thinking that's the most suitable for us atm as we're already using chakraUI - any concerns let me knowScreenshot
copiable set to true:

copied successfully:
