metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Fix Personal Sign Display

Open skgbafa opened this issue 2 years ago • 6 comments

Explanation

This PR makes a change to the personal_sign page. Currently there is an icon that is rendered based on the address passed, but the address passed is null. This PR changes the address passed to use the same value that is used to select the account (fromAccount).

More Information

Related to https://github.com/MetaMask/metamask-extension/pull/14438#issuecomment-1204225916

Screenshots/Screencaps

Before

Before

After

After

Manual Testing Steps

Go to the test-dapp and hit personal sign

Pre-Merge Checklist

  • [ ] PR template is filled out
  • [ ] IF this PR fixes a bug, a test that would have caught the bug has been added
  • [ ] PR is linked to the appropriate GitHub issue
  • [ ] PR has been added to the appropriate release Milestone

+ If there are functional changes:

  • [ ] Manual testing complete & passed
  • [ ] "Extension QA Board" label has been applied

skgbafa avatar Aug 03 '22 17:08 skgbafa

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.

github-actions[bot] avatar Aug 03 '22 17:08 github-actions[bot]

@georgewrmarshall Not sure what the identicon is supposed to show (it has always showed up blank for me) but with the context it should show the icon/ logo for the dApp requesting the signature. Doesn't make sense to show the account avatar again. We can remove it as long as the icon is displayed in the origin along with the dApp URL.

Also, this soon will be replaced by new designs @holantonela has worked on which solve these issues.

SayaGT avatar Aug 04 '22 13:08 SayaGT

We are having requesterAddress in three components set from respective containers to render the icon using <Identicon/> , SignatureRequestOriginal ConfirmEncryptionPublicKey, and ConfirmDecryptMessage, but in all three places requesterAddress is assigned to null in the respective containers. Based on the above comment by @SayaGT maybe we can remove it from all three places?

cc @danjm and @Gudahtt to know if they have more context for the use of requesterAddress.

NiranjanaBinoy avatar Aug 04 '22 16:08 NiranjanaBinoy

requesterAddress has always been null. See the original PR here. At one point it showed the Ethereum logo as a default, that must have broken at some point. @danjm might know what the original intent was, but it was a long time ago, and I'm not sure it matters.

I agree that it doesn't make sense to show the account avatar twice. Leaving this as-is or removing it would make the most sense to me for now. The new designs will certainly solve this for us.

Gudahtt avatar Aug 04 '22 18:08 Gudahtt

+1 vote for removing

georgewrmarshall avatar Aug 04 '22 19:08 georgewrmarshall

@skgbafa could you remove the renderRequestIcon as per @georgewrmarshall 's request so we can move this forward

brad-decker avatar Sep 20 '22 16:09 brad-decker

I believe we're removing this icon already in this PR: https://github.com/MetaMask/metamask-extension/pull/15776

bschorchit avatar Sep 26 '22 15:09 bschorchit

@bschorchit @georgewrmarshall is this PR still needed then? Does this mean its good to go as is?

brad-decker avatar Nov 04 '22 17:11 brad-decker

@bschorchit @georgewrmarshall is this PR still needed then? Does this mean its good to go as is?

I don't think this PR is needed. Let's prioritize https://github.com/MetaMask/metamask-extension/pull/15776 and the issue that motivated this PR won't exist anymore 🙂

bschorchit avatar Nov 04 '22 19:11 bschorchit

Closing in favor of #15776

brad-decker avatar Nov 05 '22 12:11 brad-decker