-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 group member avatar icons correctly to avoid avatar rearranging upon receiving backend response #25416
Conversation
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/libs/ReportUtils.js
Outdated
// if that still clashes, sub-sort by accountID (position 0 element in array) | ||
const sortedParticipantDetails = _.chain(participantDetails) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good to me.
Reviewer Checklist
Screenshots/VideosWebweb-group-avatar-order.movMobile Web - Chromemweb-chrome-group-avatar-order.movMobile Web - Safarimweb-safari-group-avatar-order.movDesktopdesktop-group-avatar-order.moviOSios-group-avatar.movAndroidandroid-group-avatar-order.mov |
Stuck on Android build. |
@cead22 Reviewed this. I had issues with the Android build. 🎀 👀 🎀 C+ reviewed. |
@cead22 appreciate your review on this. Thanks |
@mananjadhav I noticed the Reviewer Checklist doesn’t have “Android / native” checked although you have attached test video for it. |
I thought I had marked this explicitly. I don't know how this got missed. cc - @cead22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, but I think it's inefficient because it'll go over the array of participants 3 times every time, so I think we should update it to only go over the array once
Also, can you add a unit test for getIconsForParticipants
?
@cead22 Sure, I will update the PR and add unit test for the function |
followed by accountID efficiently add unit tests
src/libs/ReportUtils.js
Outdated
lodashGet(personalDetails, [accountID, 'firstName'], ''), | ||
avatarSource, | ||
]); | ||
participantDetails.push([accountID, lodashGet(personalDetails, [accountID, 'displayName']) || lodashGet(personalDetails, [accountID, 'login'], ''), avatarSource]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon reviewing this again, I found that the original firstName element (index 2) is only used for sorting.
The thing is, when a user has firstName configured, his displayName/login is going to be a string that starts with his firstName.
e.g.
firstName: 'Lagertha',
displayName: 'Lagertha Lothbrok',
lastName: 'Lothbrok'
So it is actually redundant to first sort using firstName
, then sub-sort using displayName/login
.
Because we will get the exact same results if we just skip sorting using firstName
, and start initial sorting using displayName/login
.
With that reasoning, I have updated the code to remove firstName element from the array, because not using it for sorting means we do not need it at all.
|
||
// Now that things are sorted, gather only the avatars (third element in the array) and return those | ||
const avatars = []; | ||
for (let i = 0; i < sortedParticipantDetails.length; i++) { | ||
const userIcon = { | ||
id: sortedParticipantDetails[i][0], | ||
source: sortedParticipantDetails[i][3], | ||
source: sortedParticipantDetails[i][2], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated from index 3 to 2 for avatar source, because the original index 2 element (firstName) is deemed unnecessary and removed.
Updated the PR, there are some changes made to sort more efficiently, and just perform the sorting using displayName/login and accountID, removing the initial sorting using firstName as that is found to be redundant, more details in the code inline #25416 (comment). Also add unit tests to validate the results. I will execute UI verification tests again and update |
@mananjadhav can you please re-review and merge if it all looks good? Thanks! |
Merged latest from main branch, rerun tests on all platforms, verified everything works well and updated @mananjadhav all yours to review and merge. Thanks. |
@mananjadhav can we get the PR reviewed and merged soon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change looks good to me.
Quick bump @cead22, but I am not sure if this is important enough to be merged during the merge-freeze. |
Sorry I was OOO last week, let me do another review and confirm whether we should hold this for the feature freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving so it's ready to merge, but holding due to feature freeze (slack thread)
@honnamkuan Can you merge the latest main? |
b99ef85
to
bcaa077
Compare
@mananjadhav unfortunately I am currently abroad and have no access to my laptop and commit signing key. Tried merge latest earlier but the checks didn’t pass due to signed commit verification, so I have reverted that. I see the PR is mergeable without conflict now, if merge latest main is needed then I will only be able to do next Thursday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing with the latest main locally, I think we can merge, but let's make this small change to make the code more readable before merging
src/libs/ReportUtils.js
Outdated
lodashGet(personalDetails, [accountID, 'firstName'], ''), | ||
avatarSource, | ||
]); | ||
participantDetails.push([accountID, lodashGet(personalDetails, [accountID, 'displayName']) || lodashGet(personalDetails, [accountID, 'login'], ''), avatarSource]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a little more readable by pulling lodashGet(personalDetails, [accountID, 'displayName']) || lodashGet(personalDetails, [accountID, 'login'], '')
into its own variable like we're doing with avatarSource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedbacks, I am back :)
Have added the const refactoring and merge latest main to my branch, done some testing and verified codes work well.
Appreciate your review again.
Tested again, works okay. @cead22 All yours. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/cead22 in version: 1.3.73-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.73-1 🚀
|
🚀 Deployed to staging by https://github.com/cead22 in version: 1.3.74-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.74-3 🚀
|
Details
sort group member avatar icons correctly using displayName/login, then sub-sort with accountID to avoid avatar rearranging upon receiving backend response
Fixed Issues
$ #23364
PROPOSAL: #23364 (comment)
Tests
Offline tests
Same as Tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-08-24.at.5.42.48.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-08-24.at.5.55.08.PM.mov
Mobile Web - Safari
Screen.Recording.2023-08-24.at.6.05.28.PM.mov
Desktop
Screen.Recording.2023-08-24.at.6.07.25.PM.mov
iOS
Screen.Recording.2023-08-24.at.6.02.32.PM.mov
Android
Screen.Recording.2023-08-24.at.5.47.23.PM.mov