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

Sort group member avatar icons correctly to avoid avatar rearranging upon receiving backend response #25416

Merged
merged 8 commits into from
Sep 22, 2023
Merged

Sort group member avatar icons correctly to avoid avatar rearranging upon receiving backend response #25416

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
990932d
sort group member avatar icons correctly
honnamkuan Aug 17, 2023
10bebee
Merge remote-tracking branch 'origin/main' into 23364
honnamkuan Aug 17, 2023
83d5baa
Merge branch 'main' into 23364
honnamkuan Aug 23, 2023
fd1f659
sort avatar icons by user displayName/login
honnamkuan Aug 23, 2023
e70c1e9
update comment
honnamkuan Aug 23, 2023
bcaa077
Merge branch 'main' into 23364
honnamkuan Aug 24, 2023
2996bf5
Merge remote-tracking branch 'origin/main' into 23364
honnamkuan Sep 20, 2023
4d93923
refactor lodashGet usages to const
honnamkuan Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,8 +874,14 @@ function getIconsForParticipants(participants, personalDetails) {
]);
}

// Sort all logins by first name (which is the second element in the array)
const sortedParticipantDetails = participantDetails.sort((a, b) => a[2] - b[2]);
// Sort all logins by first name (position 2 element in array)
// if multiple participants have the same first name, sub-sort them by login/displayName (position 1 element in array)
// if that still clashes, sub-sort by accountID (position 0 element in array)
const sortedParticipantDetails = _.chain(participantDetails)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@honnamkuan Can you confirm if chaining the sortBy won't have side effects? Also quick question, would it make sense to use this instead?

const sortedParticipantDetails = _.orderBy(participantDetails, [2, 1, 0]);

let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mananjadhav thanks for the feedbacks!
I definitely agree that _.orderBy(participantDetails, [2, 1, 0]); would looks cleaner, however _.orderBy is actually part of the Lodash utility function, it is not available within Underscore.js which Expensify App uses.

To confirm that, I tried out using orderBy and verified that I am getting Uncaught TypeError: underscore__WEBPACK_IMPORTED_MODULE_2__.default.orderBy is not a function in the console

Underscore sortBy produce stable sorted copy of array, hence no unintended side effects and can safely be included in chained processing.
I can confirm that chaining sortBy is working well, and the approach is also recommended in various SO threads when it comes to sorting using on multiple sub-elements/attributes
e.g. https://stackoverflow.com/a/35612266

Copy link
Collaborator

Choose a reason for hiding this comment

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

@honnamkuan Expensify App uses lodash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mananjadhav you are right that Expensify App uses the lodash package.
However the contributing guides STYLE.md says

We have standardized on using underscore.js methods for objects and collections

Therefore, all _ imports in the project are coming from import _ from 'underscore';
e.g.

App/src/libs/ReportUtils.js

Line 1 in c435734

import _ from 'underscore';

Having said that, I am happy to change it to use lodash orderBy if you think that is the better approach here, your advice will be much appreciated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. Thanks this works. I just went through the SO thread. I will test this shortly.

.sortBy((detail) => detail[0])
.sortBy((detail) => detail[1])
.sortBy((detail) => detail[2])
.value();

// Now that things are sorted, gather only the avatars (third element in the array) and return those
const avatars = [];
Expand Down