-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: 1273 duplicate contact addresses #7482
Conversation
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. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7482 +/- ##
==========================================
- Coverage 34.72% 34.72% -0.01%
==========================================
Files 1020 1022 +2
Lines 27252 27279 +27
Branches 2229 2232 +3
==========================================
+ Hits 9463 9472 +9
- Misses 17292 17309 +17
- Partials 497 498 +1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @frankvonhoven amazing work!
Left some comment, let me know your thoughts!!
Also, it seems that it is some UI issues on the warning on the SendTo screen? Comparatively to the other, at least doesn't seem right the margin between the two warnings for me
app/components/Views/Settings/Contacts/AmbiguousAddressSheet/AmbiguousAddressSheet.tsx
Show resolved
Hide resolved
app/components/Views/Settings/Contacts/AmbiguousAddressSheet/AmbiguousAddressSheet.tsx
Show resolved
Hide resolved
re: the warning message spacing... I did notice that but didn't know if it would be a real use case, since a user probably wouldn't want to send funds without having any funds. But perhaps I could refactor the warning component to only have a top margin? |
@frankvonhoven can you test this in dark mode as well? The caution icons look a bit dark and it would be interesting to see if it is visible in dark mode. Can you confirm that the translations are in for this feature? Confirm by showing recordings of the translated text for the new this is a ambitious address sheet In terms of analytics, can you confirm that the analytic events are appearing correctly? |
…aMask/metamask-mobile into fix/1273-duplicate-contact-addresses
@cortisiko with translations in I checked the modal in German and copied the string 3x to test long translations: |
@tommasini I fixed the padding for the warning component |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Related issues
_Fixes #1273
E2E Tests
✅ Pass: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/df69caa4-66f3-44cd-ad41-a097a6491ed7
Manual testing steps
release/7.6.0
brancha. Settings > Contacts
b. Send To --> Contacts
a. If an ambiguous address is selected, you should see a warning message on the Send To screen
Screenshots/Recordings
Contacts Ambiguous Addresses
Contacts.Ambiguous.mp4
Dark Mode
Send To Ambiguous Addresses
SelectAmbiguous.mp4
Dark Mode
Select Ambiguous Addresses to Send To
Send.To.Ambiguous.mp4
Warning Dark Mode
Pre-merge author checklist
Pre-merge reviewer checklist