fix: use the same sort order for header display names and avatars in a group chat view #28218
Conversation
|
@allroundexperts 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] |
|
@allroundexperts Bump on review, please. |
|
@akinwale Can you please resolve conflicts? |
@allroundexperts Done. |
|
@allroundexperts Bump on review, please. The PR has been up for almost a week now. |
|
Sorry, I'm on it. This has quite some file changes so I need time to test it completely. |
| const sortedDisplayNames = | ||
| optionItem.type === CONST.REPORT.TYPE.CHAT && !optionItem.isArchivedRoom && optionItem.displayNamesWithTooltips && optionItem.displayNamesWithTooltips.length > 1 | ||
| ? _.filter( | ||
| _.map(optionItem.displayNamesWithTooltips, ({displayName}) => displayName), | ||
| (displayName) => !_.isEmpty(displayName), | ||
| ).join(', ') | ||
| : ''; |
There was a problem hiding this comment.
@akinwale Can you please explain why this change is needed?
There was a problem hiding this comment.
Specifically, I'm curious about this line:
optionItem.type === CONST.REPORT.TYPE.CHAT && !optionItem.isArchivedRoom && optionItem.displayNamesWithTooltips && optionItem.displayNamesWithTooltips.length > 1
There was a problem hiding this comment.
Also, this name sortedDisplayNames seems to be confusing. optionItem.displayNamesWithTooltips are already sorted and you're just joining the array here. I think something like below makes more sense.
const fullTitle = condition ? join the display names : optionItem.text;
Doing this will also prevent the need for duplicate conditional when specifying fullTittle prop.
There was a problem hiding this comment.
In the LHN for small screen devices, for group chats, the names are not sorted by default in ascending order. The component used to display these names is DisplayNamesWithoutTooltips which makes use of the value specified for the fullTitle prop.
The conditions checked are for the following reasons:
optionItem.type === CONST.REPORT.TYPE.CHATto ensure that it is actually a group chat, not an IOU or task which uses a different string value for the title. For instance, IOU titles look likex owes y $5.00, which is different from a list of names in a group chat.!optionItem.isArchivedRoomto make sure that the room is not archived. Archived rooms use a different title format (eg.Report (archived)).- The remaining two conditions are to check to make sure that there is more than 1 participant in the report, which most likely indicates that it is a group chat.
The _.filter was added to make sure that there are no empty string display names. I added this due to one of the tests failing.
To summarise, if the report is a group chat, and not an IOU or task, and not archived, and there is more than one participant, then the list of sorted display names will be displayed as the title for devices where the DisplayNameWithoutTooltip component is rendered (usually mobile / touchscreen or small screen devices).
There was a problem hiding this comment.
Also, this name
sortedDisplayNamesseems to be confusing.optionItem.displayNamesWithTooltipsare already sorted and you're just joining the array here. I think something like below makes more sense.const fullTitle = condition ? join the display names : optionItem.text;Doing this will also prevent the need for duplicate conditional when specifying
fullTittleprop.
This won't work. displayNamesWithTooltips is a list of objects, which is the reason for the map. Also, one of the tests was failing with empty display names, which is why I added the filter.
There was a problem hiding this comment.
I mean't something else. Please read it again 😄
There was a problem hiding this comment.
Oh, right, I see it now. Assigning to a fullTitle value instead of using sortedDisplayNames.
src/libs/ReportUtils.js
Outdated
| @@ -1192,6 +1192,19 @@ function getDisplayNamesWithTooltips(personalDetailsList, isMultipleParticipantR | |||
| pronouns, | |||
| }; | |||
| }); | |||
|
|
|||
| return _.chain(displayNames) | |||
There was a problem hiding this comment.
Doesn't chain support .map? If it does, then we should chain on personalDetailsList instead.
@allroundexperts Pushed a new commit. |
src/pages/home/HeaderView.js
Outdated
| @@ -86,6 +86,8 @@ function HeaderView(props) { | |||
| const isAutomatedExpensifyAccount = ReportUtils.hasSingleParticipant(props.report) && ReportUtils.hasAutomatedExpensifyAccountIDs(participants); | |||
| const parentReportAction = ReportActionsUtils.getParentReportAction(props.report); | |||
| const isCanceledTaskReport = ReportUtils.isCanceledTaskReport(props.report, parentReportAction); | |||
| // Use sorted display names for the fullTitle instead of title for group chats on native small screen widths | |||
| const sortedDisplayNames = isMultipleParticipant ? _.map(displayNamesWithTooltips, ({displayName}) => displayName).join(', ') : ''; | |||
There was a problem hiding this comment.
Why don't we need to filter here like we did above? I think its better to create two utility functions, one called isActiveGroupChat and the other one called displayNamesFromTooltips.
There was a problem hiding this comment.
I guess in the case of the LHN, the related test specifically did not set a displayName, and there are no tests for the header view.
We only need to check if it's an active group chat in the LHN, which is done in the OptionRowLHN component, so I don't think a utility method is necessary here.
For the sorted display names joined as a string, I can add a utility method to ReportUtils for this which returns the value of:
_.filter(
_.map(displayNamesWithTooltips, ({displayName}) => displayName),
(displayName) => !_.isEmpty(displayName),
).join(', ')
Then we can use this in both HeaderView and OptionRowLHN. Agree?
There was a problem hiding this comment.
I still feel like the function to check if its a group chat would come in handy sometime later down the road. Abstracting it into a utility function doesn't hurt. Does it?
There was a problem hiding this comment.
That makes sense but my reservation was since optionItem is not actually a properly defined type (the prop type doesn't list its properties), it may be only specific to the OptionRow* components.
There was a problem hiding this comment.
Why didn't I notice that before 😵💫
You're right. It doesn't make sense to add this.
| @@ -104,6 +104,11 @@ function OptionRowLHN(props) { | |||
| const shouldShowGreenDotIndicator = | |||
| !hasBrickError && (optionItem.isUnreadWithMention || optionItem.isWaitingForTaskCompleteFromAssignee || ReportUtils.isWaitingForIOUActionFromCurrentUser(optionItem)); | |||
|
|
|||
| const fullTitle = | |||
| optionItem.type === CONST.REPORT.TYPE.CHAT && !optionItem.isArchivedRoom && optionItem.displayNamesWithTooltips && optionItem.displayNamesWithTooltips.length > 1 | |||
There was a problem hiding this comment.
lodashGet(optionItem, 'displayNamesWithTooltips', 0) > 1 doesn't work?
There was a problem hiding this comment.
Nice tip. It works. Just needed to add .length in the second parameter.
Reviewer Checklist
Screenshots/VideosWeb
Mobile Web - ChromeScreen.Recording.2023-10-02.at.2.35.41.PM.movMobile Web - SafariScreen.Recording.2023-10-02.at.2.33.01.PM.movDesktop
iOSScreen.Recording.2023-10-02.at.2.34.38.PM.movAndroid
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀
|
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.79-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
Details
Sorts the display names being displayed in the header view of a group chat using the same logic used to sort the avatars displayed in the chat view.
Fixed Issues
$ #28048
PROPOSAL: #28048 (comment)
Tests
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
28048-web.mp4
Mobile Web - Chrome
Mobile Web - Safari
Desktop
28048-desktop.mp4
iOS
Android