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

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

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

mirceanis
Copy link
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.

@mirceanis mirceanis added team-wallet-ux regression-beta-12.1.0 Regression bug that was found in beta in release 12.1.0 labels Jul 26, 2024
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.

Copy link

sonarcloud bot commented Jul 26, 2024

Copy link

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
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

@@ -47,7 +47,7 @@ describe('AddContact component', () => {
fireEvent.change(input, { target: { value: 'invalid address' } });
setTimeout(() => {
expect(getByText('Recipient address is invalid')).toBeInTheDocument();
}, 100);
}, 600);
Copy link
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
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%)

@@ -68,8 +68,10 @@ export default class AddContact extends PureComponent {
isValidHexAddress(input, { mixedCaseUseChecksum: true });
const validEnsAddress = isValidDomainName(input);

if (!IS_FLASK && !validEnsAddress && !valid) {
if (!validEnsAddress && !valid) {
Copy link
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
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
84 checks passed
@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-wallet-ux
Projects
None yet
5 participants