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

Refactor SendTo - extract code to render from / to addresses into separate component #6266

Merged
merged 14 commits into from
May 4, 2023
Merged

Refactor SendTo - extract code to render from / to addresses into separate component #6266

merged 14 commits into from
May 4, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Apr 25, 2023 •
edited
Loading

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
In this PR, we extract SendTo code logic to new components and removed dead code.
http://recordit.co/JIVVug44Rg

Test Cases

  • Should update the correct To account and From account
  • Should update account name with ens if available
  • Should show the option to add account to contact if not added
  • Should show the option to buy/transfer token is from account balance is zero

Issue

Progresses #6160

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

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

@blackdevelopa blackdevelopa marked this pull request as ready for review April 26, 2023 08:01
@blackdevelopa blackdevelopa requested a review from a team as a code owner April 26, 2023 08:01
@blackdevelopa blackdevelopa mentioned this pull request Apr 26, 2023
Closed
5 tasks
@blackdevelopa blackdevelopa self-assigned this Apr 26, 2023
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels Apr 26, 2023
@jpuri
Copy link
Contributor

jpuri commented Apr 26, 2023

Great works, just some small feedbacks 👍

digiwand
digiwand reviewed Apr 28, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand 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!

app/components/Views/SendFlow/AddressTo/SendToAddressTo.tsx Outdated Show resolved Hide resolved
app/components/Views/SendFlow/AddressTo/SendToAddressTo.tsx Outdated Show resolved Hide resolved
app/components/Views/SendFlow/SendTo/index.js Outdated Show resolved Hide resolved
digiwand
digiwand approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

Lgtm!

jpuri
jpuri reviewed May 1, 2023
View reviewed changes
ios/Podfile.lock Outdated Show resolved Hide resolved
@blackdevelopa
Copy link
Contributor Author

Hey @jpuri, checked the QR scan for SendTo and it seems to work on my end. Can you share how to reproduce what you found? Thank you

Screen_Recording_20230502-103627_MetaMask.mp4

@blackdevelopa blackdevelopa changed the title Sendto refactor Refactor SendTo - extract code to render from / to addresses into separate component May 2, 2023
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. DO-NOT-MERGE Pull requests that should not be merged and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) DO-NOT-MERGE Pull requests that should not be merged labels May 2, 2023
@seaona
Copy link
Contributor

seaona commented May 4, 2023

@blackdevelopa overall looks good. I've just found one small issue. See below

ENS name not displayed on the Confirmation screen

  • Problem: whenever we add an ENS domain as the recipient, we can see how when we land on the last Confirmation screen, the ENS is not there anymore, and we just see the resolved address instead
Screencast.from.04-05-23.12.20.29.webm
  • Expected: display ENS and address if possible (see production)

Screenshot from 2023-05-04 12-20-01

@seaona seaona removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label May 4, 2023
@seaona
Copy link
Contributor

seaona commented May 4, 2023

The issue above is not related to this PR, so I've opened a separate ticket here #6330
This PR is QA approved and can be merged.

@bschorchit @blackdevelopa

@seaona seaona added the QA Passed A successful QA run through has been done label May 4, 2023
@seaona seaona merged commit f68e629 into main May 4, 2023
@seaona seaona deleted the sendto_refactor branch May 4, 2023 12:41
@seaona seaona added the release-6.6.0 Issue or pull request that will be included in release 6.6.0 label May 4, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@digiwand digiwand digiwand approved these changes

@jpuri jpuri jpuri approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
QA Passed A successful QA run through has been done release-6.6.0 Issue or pull request that will be included in release 6.6.0 team-confirmations-secure-ux-PR PR from the confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blackdevelopa @jpuri @seaona @digiwand