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.

Sign up for GitHub

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

Jump to bottom

Added reject transactions modal to be present in the footer of the approve screen #16832

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Added reject transactions modal to be present in the footer of the approve screen #16832

merged 3 commits into from
Dec 13, 2022

Conversation

VSaric
Copy link
Contributor

@VSaric VSaric commented Dec 7, 2022

Explanation

Added reject transactions modal to be present in the footer of the approve screen. Batch reject button is present now in the token allowance screen.

Screenshots/Screencaps

Before

After

test-dapp

Screen.Recording.2022-12-07.at.11.41.34.mov

etherscan

Screen.Recording.2022-12-07.at.11.44.28.mov

Manual Testing Steps

test-dapp

  1. Navigate to the test dapp
  2. Connect your wallet
  3. Deploy the ERC20 token
  4. Click Approve Token multiple times
  5. Navigate back to the wallet

etherscan

  1. Go to https://etherscan.io/token/0x6b175474e89094c44da98b954eedeac495271d0f#writeContract
  2. Connect your wallet
  3. Under approve input any address and a number
  4. Click on write multiple times
  5. Navigate back to the wallet

@VSaric VSaric self-assigned this Dec 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [67edcd1]
Page Load Metrics (2001 ± 122 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89151110157
domContentLoaded148524251980246118
load148524252001254122
domInteractive148524251980246118
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: 462 bytes
  • common: 31 bytes

highlights:

storybook

@mirjanaKukic
Copy link
Contributor

Verified by QA

@VSaric VSaric marked this pull request as ready for review December 7, 2022 15:21
@VSaric VSaric requested a review from a team as a code owner December 7, 2022 15:21
@VSaric VSaric force-pushed the fix-reject-transactions-for-token-allowance-flow branch from 67edcd1 to e0f9dc2 Compare December 8, 2022 10:09
@bschorchit bschorchit mentioned this pull request Dec 8, 2022
Closed
@VSaric VSaric requested a review from darkwing December 8, 2022 12:27
@metamaskbot
Copy link
Collaborator

Builds ready [e0f9dc2]
Page Load Metrics (2100 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint102190127199
domContentLoaded16682372209017684
load16682381210018086
domInteractive16682372208917684
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: 498 bytes
  • common: 31 bytes

highlights:

storybook

darkwing
darkwing approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Nicely done @VSaric !

@jpuri
Copy link
Contributor

jpuri commented Dec 9, 2022

Great work 👍

Will be nice to have some test also added.

@VSaric
Copy link
Contributor Author

VSaric commented Dec 9, 2022 •
edited
Loading

Hey @jpuri, I agree on adding some test for this, but we don't have any tests for token-allowance.js. Is it okay for you that we create new ticket for adding tests for token-allowance.js and move forward with this PR and ship it?

cc: @bschorchit

@metamaskbot
Copy link
Collaborator

Builds ready [8d2f15f]
Page Load Metrics (2419 ± 149 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint982211393316
domContentLoaded205732082400296142
load205733212419311149
domInteractive205732082400296142
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: 498 bytes
  • common: 31 bytes

highlights:

storybook

@VSaric VSaric requested a review from jpuri December 12, 2022 10:09
@danjm
Copy link
Contributor

danjm commented Dec 12, 2022

Is it okay for you that we create new ticket for adding tests for token-allowance.js and move forward with this PR and ship it?

I think this would be okay in this case. This just fixes a part of the component and is sort of a blocker for next release. But we should prioritize the tests very soon.

@bschorchit
Copy link

I've created the new ticket for adding tests to token allowance here #16910 and added to PS team's board.

@PeterYinusa PeterYinusa added this to the v10.24.0 milestone Dec 12, 2022
@VSaric VSaric merged commit 11f24de into develop Dec 13, 2022
@VSaric VSaric deleted the fix-reject-transactions-for-token-allowance-flow branch December 13, 2022 12:33
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danjm danjm danjm approved these changes

@darkwing darkwing darkwing approved these changes

@pedronfigueiredo pedronfigueiredo Awaiting requested review from pedronfigueiredo pedronfigueiredo is a code owner automatically assigned from MetaMask/extension-devs

@brad-decker brad-decker Awaiting requested review from brad-decker

@NiranjanaBinoy NiranjanaBinoy Awaiting requested review from NiranjanaBinoy

@jpuri jpuri Awaiting requested review from jpuri

Assignees

@VSaric VSaric

Labels
rc-cherry-picked
Projects
None yet
Milestone
v10.24.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Users cannot reject and remove all unapproved approve transactions
9 participants
@VSaric @metamaskbot @mirjanaKukic @jpuri @danjm @bschorchit @darkwing @PeterYinusa @seaona