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

Sort contacts alphabetically #11982

Merged
merged 6 commits into from
Sep 3, 2021
Merged

Sort contacts alphabetically #11982

merged 6 commits into from
Sep 3, 2021

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Aug 31, 2021

Contacts are grouped together by letter, and the groups are listed
alphabetically, but the contacts in each group are not sorted
alphabetically themselves.

Fixes #10318.


Before

Screen Shot 2021-09-01 at 9 40 01 AM

After

Screen Shot 2021-08-31 at 5 00 49 PM

@mcmire mcmire requested a review from a team as a code owner August 31, 2021 23:05
@mcmire mcmire requested a review from adonesky1 August 31, 2021 23:05
@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.

Copy link
Contributor Author

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I tried writing an E2E test for this, but for some reason the app was misbehaving when the add contact form was opened, filled in, and then saved (the new contact was not appearing). If we feel like this is something we want to continue on to ensure proper test coverage, I may need to grab someone to help me with this.

return [
letter,
sortBy(unsortedContactsByLetter[letter], (contact) => {
return contact.name.toLowerCase();
Copy link
Contributor Author

@mcmire mcmire Aug 31, 2021

Choose a reason for hiding this comment

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

Note that each contact is sorted case-insensitively. I thought this might provide a better experience — when I look through a list that I know is sorted, I don't tend to think about case. I'm happy to change this if we think otherwise, though.

@mcmire mcmire force-pushed the sort-contacts branch 2 times, most recently from 22ebab1 to 5d6713f Compare September 1, 2021 21:21
test/jest/matchers.js Outdated Show resolved Hide resolved
ui/components/app/contact-list/contact-list.test.js Outdated Show resolved Hide resolved
Contacts are grouped together by letter, and the groups are listed
alphabetically, but the contacts in each group are not sorted
alphabetically themselves.

Fixes #10318.
@metamaskbot
Copy link
Collaborator

Builds ready [f9fe3b9]
Page Load Metrics (541 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint12268952113062
domContentLoaded3806765219847
load4006825419345
domInteractive3806755219847

.eslintrc.js Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
import React, { PureComponent } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@metamaskbot
Copy link
Collaborator

Builds ready [48967e4]
Page Load Metrics (478 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22272645610249
domContentLoaded3727164589746
load3937264789546
domInteractive3727164589746

@metamaskbot
Copy link
Collaborator

Builds ready [5e93374]
Page Load Metrics (493 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9260547211053
domContentLoaded3706034717435
load3856154937335
domInteractive3706034717435

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

looks great! thanks for the followup changes

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks @mcmire

@ryanml ryanml merged commit 49e1abc into develop Sep 3, 2021
@ryanml ryanml deleted the sort-contacts branch September 3, 2021 19:31
@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contacts are not sorted properly.
5 participants