-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Improve ENS Address Input #6606
Conversation
|
||
log.info(`ENS attempting to resolve name: ${recipient}`) | ||
this.ens.lookup(recipient.trim()) | ||
this.ens.lookup(recipient) |
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.
We use trim
here to remove the white space from copy-pastes
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.
I move it above. please see line 86
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
hey @industral @tmashuang we're reverting this in #6691 - it introduces some buggy behavior in the TO field (video below) |
@bdresser Video did not attach. |
Nevermind! It did, I guess it was added in an edit. |
As the video suggests, there's issues with changing the network while having an address or ens name. Different issues arise when going from Mainnet -> Testnet and vice-versa. I am investigating on the cause, most likely coming from the |
|
||
h(`input.send-v2__to-autocomplete__input${qrScanner ? '.with-qr' : ''}`, { | ||
placeholder: this.context.t('recipientAddress'), | ||
className: inError ? `send-v2__error-border` : '', | ||
value: to, | ||
value: recipient, |
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.
Maybe recipient || to
instead?
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.
@tmashuang Did you happen to try this out with this branch/changeset?
I believe the issue shown in the video https://cl.ly/8fb6fb8c9555, and the motivation for the revert, was an issue before this PR / before the reverted commit. There are other small bugs as Thomas mentioned. My current belief is that this PR introduced no new bugs. |
https://www.dropbox.com/s/160lnm6f21xqht9/metamask.mov?dl=0