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

#5492 Search account by name #7261

Merged
merged 10 commits into from
Dec 4, 2019

Conversation

desfero
Copy link
Contributor

@desfero desfero commented Oct 7, 2019

Adds a search field to filter accounts either by name or address. Fixes #5492 (except search by token names). But default search is hidden until the user adds 5 accounts.

It may require some 🎨design 🎨 changes but that's up to maintainers.

Demo
Oct-08-2019 08-49-01

ui/app/selectors/selectors.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@desfero desfero left a comment

Choose a reason for hiding this comment

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

Found a bug with renderScrollButton, will fix tomorrow, hold on with the review, please.

@desfero
Copy link
Contributor Author

desfero commented Oct 8, 2019

@Gudahtt ready for a review again

hubertglowiak
hubertglowiak previously approved these changes Oct 16, 2019
@cjeria
Copy link
Contributor

cjeria commented Oct 23, 2019

Cool feature! I'd suggest adding a little more padding (8px) inside the search field (we are using a 4px spacing system for our UI - check it out here) and a search icon for better affordance.

Suggestion - I think we should add a conditional that would hide this feature if the user has less than 4 accounts. Having this feature on by default would add unnecessary clutter to this view. I would also suggest adding a switch in settings to turn in on/off (off by default though and on if > 4 accounts).

@desfero
Copy link
Contributor Author

desfero commented Oct 23, 2019

Hey @cjeria thanks for a review, it's already hidden until we reach a point of 5 accounts.

I will do UI changes today. But do we really need some switch to turn it off? Isn't it enough to just hide if there are fewer than 5 accounts (like we do now)?

@cjeria
Copy link
Contributor

cjeria commented Oct 24, 2019

I'm just noticing you mentioned the conditional in your comment, sorry about that! Let's try this feature without the switch and we'll keep an eye out for any feedback from our users @desfero. Could also you post another screenshot when you've made those changes, please?

@desfero
Copy link
Contributor Author

desfero commented Nov 2, 2019

Hey @cjeria, sorry for taking so long, here is the screenshot with updated padding to 8px
image
Let me know if it's ok please.

@desfero desfero force-pushed the feature/accounts-search branch from e09872a to 7210690 Compare November 2, 2019 15:58
Gudahtt
Gudahtt previously approved these changes Nov 6, 2019
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks good! There are a couple more changes I'd like, but nothing important (e.g. the comment in selectors.js is still unaddressed)

app/_locales/en/messages.json Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Nov 7, 2019
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me.

ui/app/selectors/selectors.js Outdated Show resolved Hide resolved
@cjeria
Copy link
Contributor

cjeria commented Nov 13, 2019

Sorry for the delayed response @desfero

The new spacing definitely looks better! For added polish, I had suggested including a search icon to make it clear there's a search bar up there (see screenshot).

So my suggested improvements are:

  • Add search icon (grey by default, white on focus)
  • Make bottom highlight (bottom border) color to white #FFFFFF

image

@Gudahtt
Copy link
Member

Gudahtt commented Nov 21, 2019

Hey @desfero - would you prefer one of us implement these last couple of changes? I'd be happy to finish this off if you're busy.

@desfero
Copy link
Contributor Author

desfero commented Nov 22, 2019

Hey @Gudahtt last week for me was quite busy with a lot of high priority work, I will try to add a search icon today or tomorrow.

@Gudahtt
Copy link
Member

Gudahtt commented Nov 22, 2019

Sure, no rush! Just wanted to offer help, as I'm looking forward to seeing this merged.

@cjeria
Copy link
Contributor

cjeria commented Nov 23, 2019

If it makes it any easier, we've been using free fontawesome icons in the product. Here's a link to the regular weight option if you'd like to go this route https://fontawesome.com/icons/search?style=regular

@desfero
Copy link
Contributor Author

desfero commented Nov 23, 2019

@cjeria I have used one that was already used somewhere else (probably token search). Here how it looks right now:
image

@cjeria @Gudahtt let me know if it's okay for you both :)

@cjeria
Copy link
Contributor

cjeria commented Nov 23, 2019

LGTM!

@Gudahtt Gudahtt force-pushed the feature/accounts-search branch from 2b0ae74 to 4c2d36f Compare December 3, 2019 20:52
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @desfero !

I've pulled out a couple of things into separate PRs (#7626 and #7628 ), and I've rebased this against develop. I think this is ready to merge now.

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

This is a solid addition, thanks @desfero! (And thanks @Gudahtt for jumping in as well!)

@whymarrh whymarrh merged commit 638e242 into MetaMask:develop Dec 4, 2019
@desfero desfero deleted the feature/accounts-search branch December 4, 2019 03:07
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.

Search account by name
5 participants