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

Account Icon matches User's Preferred Identicon #6216

Merged
merged 1 commit into from
Apr 19, 2023
Merged

Account Icon matches User's Preferred Identicon #6216

merged 1 commit into from
Apr 19, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Apr 19, 2023

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
The account icon should match the user's preferred identicon. See issue

Steps:

  • Go to metamask test dapp
  • Send an EIP1559 transaction
  • Notice that the account icon is not the same as the sender icon (still it's the same account)

Screenshots/Recordings
Before:
image

After: http://recordit.co/vOtElnCTgq

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/894

Checklist

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

@blackdevelopa blackdevelopa requested a review from a team as a code owner April 19, 2023 12:29
@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 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 19, 2023
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.

approving with the notion that we will move logic from ApproveTransactionHeader into the domain of AccountBalance or AccountBase in a future PR

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.

👍

@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Priority - High Task with high priority and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 19, 2023
@blackdevelopa blackdevelopa self-assigned this Apr 19, 2023
@seaona seaona added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 19, 2023
@seaona
Copy link
Contributor

seaona commented Apr 19, 2023

The fix looks good. Now the Identicon is matching the selected preference from the user. This is displayed on the 2 duplicated From fields, but consolidating the design will be solved in a separate issue,as noticed here #6163

The PR is good to merge

mobile-identicon-fixed.mp4

@seaona seaona merged commit 0e694b8 into main Apr 19, 2023
13 checks passed
@seaona seaona deleted the bgfx/894 branch April 19, 2023 15:25
@github-actions github-actions bot locked and limited conversation to collaborators Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Priority - High Task with high priority QA Passed A successful QA run through has been done release-6.5.0 team-confirmations-secure-ux-PR PR from the confirmations team unit test coverage confirmed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants