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

Show transaction error message tooltips for statuses #5128

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

whymarrh
Copy link
Contributor

Fixes #3768

This PR updates the TransactionStatus component to accept an optional title prop and the TransactionListItem to use the transaction's error message as the title if it exists.

Pics or it didn't happen

In the best case a new txMeta will have the RPC error message directly on it:

screen shot 2018-08-22 at 21 05 19

If there's no error we'll have just the status:

screen shot 2018-08-22 at 21 22 34

In the worst case, we're showing this monstrosity:

screen shot 2018-08-22 at 21 22 22

Initially I didn't want to have txMeta.err.message in the title but in some (non-RPC) cases that's the best we've got and it contains useful information.

If the tx doesn't have a human readable RPC error messgae we can default to
the `err.message` value (or nothing if it's not defined). Some of the non-RPC
errors will have useful stringified values as their message despite the RPC
errors having atrocious error messages.
@whymarrh whymarrh requested review from alextsg and frankiebee August 22, 2018 23:55
@whymarrh whymarrh requested a review from danjm as a code owner August 22, 2018 23:55
@whymarrh whymarrh merged this pull request into MetaMask:refactor-tx-list Aug 23, 2018
@whymarrh whymarrh deleted the tx-error-msg branch August 23, 2018 00:11
@bdresser
Copy link
Contributor

@whymarrh possible to use the nice black tooltips we have in the extension? just asking cuz I know christian gonna ask when he sees it 😬

@whymarrh
Copy link
Contributor Author

@bdresser the thinking was that the new UI uses the black tooltips for actions/buttons and where these tooltips are more informational that it wouldn't be a good fit—good point though, we'll run it by Christian in next design chat and update it

@bdresser
Copy link
Contributor

I know that he wanted to use the black tooltip to explain the "pending" state (MetaMask/Design#25 (comment)) so probably would work for all of em -- we can double check in design sync tomorrow

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