Skip to content

fix: enable Save button on Add Contact page for address input#26155

Merged
darkwing merged 2 commits intodevelopfrom
bugfix/25918-add-contact-save-button-disabled
Aug 6, 2024
Merged

fix: enable Save button on Add Contact page for address input#26155
darkwing merged 2 commits intodevelopfrom
bugfix/25918-add-contact-save-button-disabled

Conversation

@mirceanis
Copy link
Copy Markdown
Contributor

@mirceanis mirceanis commented Jul 26, 2024

Description

This patches an issue in add-contact.component.js where the disabled state of the Save button would disappear only after a successful ENS resolution, effectively preventing plain addresses to be entered.
I also added some extra unit tests to check for some of the cases that weren't covered before.

Open in GitHub Codespaces

Related issues

fixes #25918
fixes #25889

Manual testing steps

  1. Go to Settings > Contacts > Add Contact
  2. Enter a name and a valid Ethereum address
  3. Observe the Save button getting enabled
  4. Edit the address to make it invalid, observe the Save button getting disabled
  5. Instead of an address, enter an ENS name and pick the resolved result
  6. Observe the Save button getting enabled
  7. Observe that the input is still editable (repeat 4.)

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

fixes #25918
fixes #25889

Signed-off-by: Mircea Nistor <mirceanis@gmail.com>
@mirceanis mirceanis added team-core-extension-ux Core Extension UX team regression-beta-12.1.0 Regression bug that was found in beta in release 12.1.0 labels Jul 26, 2024
@github-actions
Copy link
Copy Markdown
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.

Signed-off-by: Mircea Nistor <mirceanis@gmail.com>
@sonarqubecloud
Copy link
Copy Markdown

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 69.94%. Comparing base (f9f3cff) to head (92d190c).
Report is 69 commits behind head on develop.

Files Patch % Lines
...tact-list-tab/add-contact/add-contact.component.js 50.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26155      +/-   ##
===========================================
+ Coverage    69.70%   69.94%   +0.24%     
===========================================
  Files         1409     1409              
  Lines        49788    49797       +9     
  Branches     13768    13771       +3     
===========================================
+ Hits         34702    34828     +126     
+ Misses       15086    14969     -117     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines -153 to +159
selectedAddress: resolvedAddress,
input: resolvedAddress,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This leaves the input field editable after an ENS resolved address has been picked

setTimeout(() => {
expect(getByText('Recipient address is invalid')).toBeInTheDocument();
}, 100);
}, 600);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the debounce timeout is 500ms

@mirceanis mirceanis marked this pull request as ready for review July 26, 2024 13:45
@mirceanis mirceanis requested a review from a team as a code owner July 26, 2024 13:45
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [92d190c]
Page Load Metrics (154 ± 179 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6812998178
domContentLoaded95625147
load391781154374179
domInteractive85625147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -4 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

const validEnsAddress = isValidDomainName(input);

if (!IS_FLASK && !validEnsAddress && !valid) {
if (!validEnsAddress && !valid) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we not need it for FLASK?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it doesn't matter if we are in FLASK or not, the validation is the same

@darkwing darkwing merged commit 59cee4e into develop Aug 6, 2024
@darkwing darkwing deleted the bugfix/25918-add-contact-save-button-disabled branch August 6, 2024 19:59
@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 6, 2024
@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

regression-beta-12.1.0 Regression bug that was found in beta in release 12.1.0 regression-RC-12.1.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-core-extension-ux Core Extension UX team

Projects

None yet

5 participants