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

Fix/2830 enter many names #4089

Merged
merged 22 commits into from
Apr 20, 2022
Merged

Fix/2830 enter many names #4089

merged 22 commits into from
Apr 20, 2022

Conversation

tommasini
Copy link
Contributor

Description

The input text is not longer transformed in a formatted text with was impossible to keep writing.
Now it is a text input with a check icon that tell us that the address it is valid.

Checklist

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

Issue

Resolves #2830
Resolves the fact that we could not write many names like "evan.ethereum.ether" and now we can access that addresses too with this solution.

Small demo with the final result

Simulator.Screen.Recording.-.iPhone.11.Pro.-.2022-03-29.at.10.52.58.mp4

tommasini and others added 13 commits March 28, 2022 19:11
@tommasini tommasini requested a review from a team as a code owner April 12, 2022 14:46
@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.

@tommasini tommasini mentioned this pull request Apr 12, 2022
Closed
3 tasks
@chrisleewilcox chrisleewilcox added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 12, 2022
@chrisleewilcox
Copy link
Contributor

When an addressed is resolved, should we not show the warning to confirm it's a valid address? Since the address is resolved? Should the warning only show for addresses and names not resolved?
image.png

@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Apr 12, 2022
chrisleewilcox
chrisleewilcox requested changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Fixed detox addressbook-tests.spec.js that broke with this branch.

chrisleewilcox
chrisleewilcox requested changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Fixed detox addressbook-tests.spec.js that broke with this branch.

@chrisleewilcox chrisleewilcox added the QA'd but questions A QA run through has been done but you need clarification on minor issues you found label Apr 13, 2022
chrisleewilcox
chrisleewilcox approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

LGTM

gantunesr
gantunesr requested changes Apr 13, 2022
View reviewed changes
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

Good work man, just a few comments

  • Check why ENS is showing instead of address
  • Can you verify that this flow is working well with ERC721 (NFTs)

app/components/Views/SendFlow/AddressInputs/index.js Show resolved Hide resolved
app/components/Views/SendFlow/AddressInputs/index.js Outdated Show resolved Hide resolved
app/components/Views/SendFlow/Confirm/index.js Outdated Show resolved Hide resolved
app/components/Views/SendFlow/AddressInputs/index.js Outdated Show resolved Hide resolved
app/components/Views/SendFlow/AddressInputs/index.js Outdated Show resolved Hide resolved
app/util/address/index.js Outdated Show resolved Hide resolved
app/util/address/index.test.ts Outdated Show resolved Hide resolved
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd but questions A QA run through has been done but you need clarification on minor issues you found QA in Progress QA has started on the feature. QA Passed A successful QA run through has been done labels Apr 14, 2022
@tommasini tommasini mentioned this pull request Apr 14, 2022
Closed
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 15, 2022
@chrisleewilcox
Copy link
Contributor

image.png

@chrisleewilcox
Copy link
Contributor

image.png

@chrisleewilcox
Copy link
Contributor

image.png

chrisleewilcox
chrisleewilcox approved these changes Apr 18, 2022
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Retested and tested confirm screen with screenshots provided.

LGTM

@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Apr 18, 2022
gantunesr
gantunesr approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

@tommasini tommasini merged commit 620bd6e into main Apr 20, 2022
@tommasini tommasini deleted the fix/2830-enter-many-names branch April 20, 2022 20:42
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chrisleewilcox chrisleewilcox chrisleewilcox approved these changes

@gantunesr gantunesr gantunesr approved these changes

Assignees

@tommasini tommasini

Labels
QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Immediately resolves name as soon as I enter ".eth", but this excludes the ability to enter many names
3 participants
@tommasini @chrisleewilcox @gantunesr