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 notification for dropped retry transactions #4363

Merged
merged 1 commit into from
May 29, 2018
Merged

Conversation

alextsg
Copy link
Contributor

@alextsg alextsg commented May 24, 2018

Fixes #4018, changes some styling/wording related to #3885

This PR adds a popup to be shown when the user is retrying a previous transaction while that previous transaction is confirmed. This PR also introduces using optional params in the URL - when retrying a transaction the new transaction's ID will be used as a param so that we can avoid using index to retrieve the transaction object. If no param is present then we fall back to index.

@cjeria
image
image
image
image

@metamaskbot
Copy link
Collaborator

Builds ready [f2eb35a]: mascara, chrome, firefox, edge, opera

@cjeria
Copy link
Contributor

cjeria commented May 24, 2018

@alextsg Looks good. However, on the second screenshot above, the "speed up" screen should still be behind the alert dialog. Looks like it disappears from the view?

@danjm
Copy link
Contributor

danjm commented May 25, 2018

Code looks good, except that I am reminded that we really need to eliminate the code duplication between the confirm components.

I'll approve after the changes needed to address Christian's comment above.

@alextsg
Copy link
Contributor Author

alextsg commented May 25, 2018

@cjeria After diving into the confirm transaction code more, I don't think there's currently a quick way for us to keep the data for the confirm screen without possibly introducing other concerns. We'll definitely address this in the task to refactor the confirm transaction flow and make sure your concerns are resolved when that's done.

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.

6 participants