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

Remove new address alert #14811

Merged
merged 18 commits into from Dec 8, 2022
Merged

Remove new address alert #14811

merged 18 commits into from Dec 8, 2022

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Jun 1, 2022

Fixes: #14783

Explanation

See background in issue

Manual Testing Steps

  1. Go through the send flow attempting to send to an account that is not in your address book
  2. Ensure the New address detected notification does not show (See videos in Issue)

@ryanml ryanml requested a review from a team as a code owner June 1, 2022 05:31
@ryanml ryanml self-assigned this Jun 1, 2022
@ryanml ryanml force-pushed the fix-14783 branch 2 times, most recently from 768dc3e to 8cbf002 Compare June 1, 2022 06:44
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Tested sending Rinkeby eth to new address. LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [afff7c7]
Page Load Metrics (1833 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint771630187331159
domContentLoaded16362181181518488
load16432182183317785
domInteractive16362181181518488

highlights:

storybook

Copy link

@Nunnaphat1234 Nunnaphat1234 left a comment

Choose a reason for hiding this comment

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

Hi

@MetaMask MetaMask deleted a comment from Nunnaphat1234 Jun 3, 2022
@MetaMask MetaMask deleted a comment from Nunnaphat1234 Jun 3, 2022
segun
segun previously approved these changes Jun 8, 2022
Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

I tested this and I can confirm the Add contact dialog is no longer shown

@darkwing
Copy link
Contributor

darkwing commented Jun 8, 2022

Tested and works well but I think we should add some padding here:
NoPadding

@darkwing darkwing dismissed stale reviews from ghost and segun via e8d4e3f June 14, 2022 14:07
@github-actions
Copy link
Contributor

github-actions bot commented Jun 14, 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.

@darkwing
Copy link
Contributor

darkwing commented Jun 14, 2022

Took the opportunity to update the padding:
PAddingFix

Also removed an unused prop

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented Jun 15, 2022

Further riffing on this. To remove assumed padding of child and to clean up the jsx a bit

Before

Screen Shot 2022-06-15 at 2 21 43 PMScreen Shot 2022-06-15 at 2 21 04 PM

After

Screen Shot 2022-06-15 at 2 16 32 PMScreen Shot 2022-06-15 at 2 18 03 PM

brad-decker
brad-decker previously approved these changes Jul 12, 2022
darkwing
darkwing previously approved these changes Jul 12, 2022
@danjm danjm added the PR - P1 identifies PRs deemed priority for Extension team label Jul 20, 2022
@bschorchit
Copy link

Noticed this PR got the two needed approvals, but ended up not being merged due to conflicts. Should we resolve them? cc: @ryanml

@pedronfigueiredo
Copy link
Contributor

I ran into an interesting issue on edit-gas-fee.spec.js, coming from the NumericInput component inside FormField, inside base-fee-input.

Decimal separators , or . cannot be written at the end of the input field. Doing so causes the input field to clear.

Writing the , separator in the middle of the number of characters works.

Since this is unrelated to this branch's change (develop is similarly broken), I modified said broken e2e test case so that it wouldn't fail. The bug is captured on a separate issue (#16703), as part of which it should be fixed and tested.

@metamaskbot
Copy link
Collaborator

Builds ready [e0da180]
Page Load Metrics (2013 ± 189 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint911811152612
domContentLoaded151129832004392188
load151129832013393189
domInteractive151129832004392188
Bundle size diffs
  • background: 0 bytes
  • ui: -1880 bytes
  • common: 0 bytes

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [8601aac]
Page Load Metrics (2089 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1041891292211
domContentLoaded160325372084215103
load16192537208920799
domInteractive160325372084215103
Bundle size diffs
  • background: 0 bytes
  • ui: -1984 bytes
  • common: 0 bytes

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [4004d61]
Page Load Metrics (2102 ± 119 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint92151111168
domContentLoaded154625042067242116
load154625292102249119
domInteractive154625042067242116
Bundle size diffs
  • background: 0 bytes
  • ui: -1984 bytes
  • common: 0 bytes

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [152ed99]
Page Load Metrics (2196 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1012411403215
domContentLoaded171624992180211101
load17162499219620699
domInteractive171624992180211101
Bundle size diffs
  • background: 0 bytes
  • ui: -1984 bytes
  • common: 0 bytes

highlights:

storybook

Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Nice cleanup 👍

@metamaskbot
Copy link
Collaborator

Builds ready [c0e34a0]
Page Load Metrics (2148 ± 106 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1042721423818
domContentLoaded172424742118228109
load172424742148220106
domInteractive172424742118228109
Bundle size diffs
  • background: 0 bytes
  • ui: -1984 bytes
  • common: 0 bytes

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [5583dda]
Page Load Metrics (2241 ± 130 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1011731282211
domContentLoaded167228522211255123
load167229612241271130
domInteractive167228522211255122
Bundle size diffs
  • background: 0 bytes
  • ui: -1984 bytes
  • common: 0 bytes

highlights:

storybook

@pedronfigueiredo pedronfigueiredo merged commit a759d42 into develop Dec 8, 2022
@pedronfigueiredo pedronfigueiredo deleted the fix-14783 branch December 8, 2022 14:20
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR - P1 identifies PRs deemed priority for Extension team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove new address alert