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

feat: Select token supports uppercase search & Support symbol search #325

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

nlyrthiia
Copy link

@nlyrthiia nlyrthiia commented Nov 27, 2022

Description of the Changes

When entering 'ETH', 'usdc' in the search box, the correct results cannot be obtained. After the modification, the uppercase can also be used to search for the correct results and the search for the symbol field has been increased. There is also a discussion about whether spaces should be included in the search. Referring to uniswap, both upper and lower case can be searched normally, spaces are not in the search field, and only symbols are retrieved.

change:

Screencast.from.27-11-22.09.37.54.webm

origin:

Screencast.from.27-11-22.09.35.47.webm

uniswap:

Screencast.from.27-11-22.09.38.21.webm

Checklist

  • Manually tests of the main Application flows are done and passed.
  • New unit / functional tests have been added (whenever applicable).
  • Docs have been updated (whenever relevant).
  • PR title is follow the Conventional Commits convention: <type>[optional scope]: <description>, e.g: fix: prevent racing of requests

This change is Reviewable

const results = tokens.filter(token => token.name.toLowerCase().includes(searchTerm));
const results = tokens.filter(token => {
const {name, symbol} = token;
const searchTermValue = searchTerm.toLowerCase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can move this line outside of the filter iterator?

@dan-ziv
Copy link
Collaborator

dan-ziv commented Nov 27, 2022

Hey @nlyrthiia, thanks for this PR - good idea :-)
Left some small comment, after this we can merge.

@nlyrthiia
Copy link
Author

nlyrthiia commented Nov 29, 2022

@dan-ziv ok gratitude for the suggestion, the code has been modified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants