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

Add Copy Tx ID button to transaction-list-item-details #6146

Merged
merged 2 commits into from
Feb 17, 2019

Conversation

chikeichan
Copy link
Contributor

copy

@bdresser @cjeria
See attached gif. I added a "copy to clipboard" to tx detail for user to copy their tx id. I thought this fits the existing UX better than adding another component showing the tx hash. What do you think?

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

One small thing


this.setState({ justCopied: true }, () => copyToClipboard(hash))

setTimeout(() => this.setState({ justCopied: false }), 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this into the callback for the above setState call? We'd want this to run after the copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea make sense, i will move it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@bdresser
Copy link
Contributor

bdresser commented Feb 14, 2019

LGTM @chikeichan -- does this work out okay when we're also showing the "Cancel" and "Speed Up" buttons?

danjm
danjm previously approved these changes Feb 15, 2019
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

I think this is good to go, once Whymarrh's comment is addressed.

@damster7

This comment has been minimized.

@damster7

This comment has been minimized.

@whymarrh whymarrh merged commit b58a9bd into MetaMask:develop Feb 17, 2019
@danfinlay danfinlay mentioned this pull request Feb 19, 2019
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.

5 participants