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

NFT Approval and SetApprovalForAll confirmation screens header updated #15727

Merged
merged 8 commits into from
Sep 21, 2022
Merged

NFT Approval and SetApprovalForAll confirmation screens header updated #15727

merged 8 commits into from
Sep 21, 2022
+57 −24

Conversation

filipsekulic
Copy link
Contributor

@filipsekulic filipsekulic commented Aug 30, 2022 •
edited
Loading

Explanation

Updated the NFT Approval and SetApprovalForAll confirmation screens header.

More Information

Screenshots/Screencaps

Before

Before.mov

After

After.mov

Manual Testing Steps

1st case:

  1. Open the test dapp https://metamask.github.io/test-dapp/
  2. Under the "Collectibles" section, click on "Deploy"
  3. Wait for transaction to be confirmed and click on "Mint"
  4. Wait for transaction to be confirmed and click on "Approve" to get our NFT approval confirmation screen
  5. Click on "Set Approval For All" to get our setApprovalForAll confirmation screen

2nd case:

  1. Open the test dapp https://metamask.github.io/test-dapp/
  2. Under the "Send tokens" section, click on "Create token"
  3. Wait for the token to be created and click on "Approve tokens" or "Approve tokens without gas"

@filipsekulic filipsekulic self-assigned this Aug 30, 2022
@github-actions
Copy link
Contributor

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.

@mirjanaKukic
Copy link
Contributor

Verified by QA

@filipsekulic filipsekulic marked this pull request as ready for review August 31, 2022 09:38
@filipsekulic filipsekulic requested a review from a team as a code owner August 31, 2022 09:38
@filipsekulic filipsekulic requested a review from danjm August 31, 2022 09:38
@filipsekulic filipsekulic force-pushed the update-nft-approval-and-set-approval-for-all-header branch 2 times, most recently from b6c4124 to 387e65a Compare September 8, 2022 14:06
@metamaskbot
Copy link
Collaborator

Builds ready [f2bf921]
Page Load Metrics (1834 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97173120189
domContentLoaded16292115181013263
load16522115183413464
domInteractive16292114181013263

highlights:

storybook

@mirjanaKukic
Copy link
Contributor

Verified by QA

@adonesky1
Copy link
Contributor

Do we want this header show up for ERC20 approvals too? Probably right?

Screen.Recording.2022-09-09.at.11.24.12.AM.mov

adonesky1
adonesky1 requested changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

Seems like we want to make this change for ERC20 approvals too?

@metamaskbot
Copy link
Collaborator

Builds ready [9b02d25]
Page Load Metrics (1710 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89170115199
domContentLoaded15732132168812158
load15732132171012259
domInteractive15732132168812158

highlights:

storybook

@filipsekulic
Copy link
Contributor Author

filipsekulic commented Sep 12, 2022 •
edited
Loading

Seems like we want to make this change for ERC20 approvals too?

Yes, I had checked it previously with @bschorchit before I made changes here (45ebf6c) and updated the screencast (the after one).

@filipsekulic filipsekulic mentioned this pull request Sep 12, 2022
Merged
@metamaskbot
Copy link
Collaborator

Builds ready [45ebf6c]
Page Load Metrics (1940 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint992080223426205
domContentLoaded16312323191817383
load16312323194017584
domInteractive16312323191817383

highlights:

storybook

@adonesky1
Copy link
Contributor

@filipsekulic need a rebase here and then looks good to go!

@filipsekulic filipsekulic force-pushed the update-nft-approval-and-set-approval-for-all-header branch from 45ebf6c to f09d606 Compare September 15, 2022 19:48
@filipsekulic
Copy link
Contributor Author

@filipsekulic need a rebase here and then looks good to go!

Done ✅

@metamaskbot
Copy link
Collaborator

Builds ready [b66f487]
Page Load Metrics (1513 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1072141393014
domContentLoaded12621769150115172
load12621869151315976
domInteractive12621769150115172

highlights:

storybook

adonesky1
adonesky1 previously approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Filip!

@filipsekulic filipsekulic force-pushed the update-nft-approval-and-set-approval-for-all-header branch from b66f487 to 8a85f7b Compare September 21, 2022 12:07
@metamaskbot
Copy link
Collaborator

Builds ready [8a85f7b]
Page Load Metrics (1215 ± 27 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint861172153234112
domContentLoaded1113136712015727
load1113136712155527
domInteractive1113136712015727

highlights:

storybook

@adonesky1 adonesky1 merged commit 2ba6e68 into develop Sep 21, 2022
@adonesky1 adonesky1 deleted the update-nft-approval-and-set-approval-for-all-header branch September 21, 2022 14:15
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adonesky1 adonesky1 adonesky1 approved these changes

@brad-decker brad-decker brad-decker approved these changes

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

Assignees

@filipsekulic filipsekulic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update the NFT Approval and SetApprovalForAll confirmation screens to use the new header component
5 participants
@filipsekulic @mirjanaKukic @metamaskbot @adonesky1 @brad-decker