Skip to content

[CP Staging] Revert "fix: use the same sort order for header display names and avatars in a group chat view" #28929

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

Merged
srikarparsi merged 1 commit into from
Oct 5, 2023
Merged

[CP Staging] Revert "fix: use the same sort order for header display names and avatars in a group chat view" #28929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 1 addition & 6 deletions src/components/LHNOptionsList/OptionRowLHN.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,6 @@ function OptionRowLHN(props) {
const shouldShowGreenDotIndicator =
!hasBrickError && (optionItem.isUnreadWithMention || optionItem.isWaitingForTaskCompleteFromAssignee || ReportUtils.isWaitingForIOUActionFromCurrentUser(optionItem));

const fullTitle =
optionItem.type === CONST.REPORT.TYPE.CHAT && !optionItem.isArchivedRoom && lodashGet(optionItem, 'displayNamesWithTooltips.length', 0) > 1
? ReportUtils.getDisplayNamesStringFromTooltips(optionItem.displayNamesWithTooltips)
: optionItem.text;

/**
* Show the ReportActionContextMenu modal popover.
*
Expand Down Expand Up @@ -227,7 +222,7 @@ function OptionRowLHN(props) {
<View style={[styles.flexRow, styles.alignItemsCenter, styles.mw100, styles.overflowHidden]}>
<DisplayNames
accessibilityLabel={translate('accessibilityHints.chatUserDisplayNames')}
fullTitle={fullTitle}
fullTitle={optionItem.text}
displayNamesWithTooltips={optionItem.displayNamesWithTooltips}
tooltipEnabled
numberOfLines={1}
Expand Down
62 changes: 18 additions & 44 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,50 +1191,25 @@ function getDisplayNameForParticipant(accountID, shouldUseShortForm = false) {
* @returns {Array}
*/
function getDisplayNamesWithTooltips(personalDetailsList, isMultipleParticipantReport) {
return _.chain(personalDetailsList)
.map((user) => {
const accountID = Number(user.accountID);
const displayName = getDisplayNameForParticipant(accountID, isMultipleParticipantReport) || user.login || '';
const avatar = UserUtils.getDefaultAvatar(accountID);

let pronouns = user.pronouns;
if (pronouns && pronouns.startsWith(CONST.PRONOUNS.PREFIX)) {
const pronounTranslationKey = pronouns.replace(CONST.PRONOUNS.PREFIX, '');
pronouns = Localize.translateLocal(`pronouns.${pronounTranslationKey}`);
}

return {
displayName,
avatar,
login: user.login || '',
accountID,
pronouns,
};
})
.sort((first, second) => {
// First sort by displayName/login
const displayNameLoginOrder = first.displayName.localeCompare(second.displayName);
if (displayNameLoginOrder !== 0) {
return displayNameLoginOrder;
}

// Then fallback on accountID as the final sorting criteria.
return first.accountID > second.accountID;
})
.value();
}
return _.map(personalDetailsList, (user) => {
const accountID = Number(user.accountID);
const displayName = getDisplayNameForParticipant(accountID, isMultipleParticipantReport) || user.login || '';
const avatar = UserUtils.getDefaultAvatar(accountID);

let pronouns = user.pronouns;
if (pronouns && pronouns.startsWith(CONST.PRONOUNS.PREFIX)) {
const pronounTranslationKey = pronouns.replace(CONST.PRONOUNS.PREFIX, '');
pronouns = Localize.translateLocal(`pronouns.${pronounTranslationKey}`);
}

/**
* Gets a joined string of display names from the list of display name with tooltip objects.
*
* @param {Object} displayNamesWithTooltips
* @returns {String}
*/
function getDisplayNamesStringFromTooltips(displayNamesWithTooltips) {
return _.filter(
_.map(displayNamesWithTooltips, ({displayName}) => displayName),
(displayName) => !_.isEmpty(displayName),
).join(', ');
return {
displayName,
avatar,
login: user.login || '',
accountID,
pronouns,
};
});
}

/**
Expand Down Expand Up @@ -3839,7 +3814,6 @@ export {
getIcons,
getRoomWelcomeMessage,
getDisplayNamesWithTooltips,
getDisplayNamesStringFromTooltips,
getReportName,
getReport,
getReportIDFromLink,
Expand Down
3 changes: 1 addition & 2 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ function HeaderView(props) {
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(props.report);
const isTaskReport = ReportUtils.isTaskReport(props.report);
const reportHeaderData = !isTaskReport && !isChatThread && props.report.parentReportID ? props.parentReport : props.report;
// Use sorted display names for the title for group chats on native small screen widths
const title = isMultipleParticipant ? ReportUtils.getDisplayNamesStringFromTooltips(displayNamesWithTooltips) : ReportUtils.getReportName(reportHeaderData);
const title = ReportUtils.getReportName(reportHeaderData);
const subtitle = ReportUtils.getChatRoomSubtitle(reportHeaderData);
const parentNavigationSubtitleData = ReportUtils.getParentNavigationSubtitle(reportHeaderData);
const isConcierge = ReportUtils.hasSingleParticipant(props.report) && _.contains(participants, CONST.ACCOUNT_ID.CONCIERGE);
Expand Down
52 changes: 26 additions & 26 deletions tests/unit/ReportUtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,12 @@ describe('ReportUtils', () => {
test('withSingleParticipantReport', () => {
expect(ReportUtils.getDisplayNamesWithTooltips(participantsPersonalDetails, false)).toStrictEqual([
{
displayName: '(833) 240-3627',
displayName: 'Ragnar Lothbrok',
login: 'ragnar@vikings.net',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
},
login: '+18332403627@expensify.sms',
accountID: 4,
accountID: 1,
pronouns: undefined,
},
{
Expand All @@ -139,6 +139,15 @@ describe('ReportUtils', () => {
accountID: 3,
pronouns: 'She/her',
},
{
displayName: '(833) 240-3627',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
login: '+18332403627@expensify.sms',
accountID: 4,
pronouns: undefined,
},
{
displayName: 'Lagertha Lothbrok',
avatar: {
Expand All @@ -148,27 +157,18 @@ describe('ReportUtils', () => {
accountID: 5,
pronouns: 'She/her',
},
{
displayName: 'Ragnar Lothbrok',
login: 'ragnar@vikings.net',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
},
accountID: 1,
pronouns: undefined,
},
]);
});

test('withMultiParticipantReport', () => {
expect(ReportUtils.getDisplayNamesWithTooltips(participantsPersonalDetails, true)).toStrictEqual([
{
displayName: '(833) 240-3627',
displayName: 'Ragnar',
login: 'ragnar@vikings.net',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
},
login: '+18332403627@expensify.sms',
accountID: 4,
accountID: 1,
pronouns: undefined,
},
{
Expand All @@ -189,6 +189,15 @@ describe('ReportUtils', () => {
accountID: 3,
pronouns: 'She/her',
},
{
displayName: '(833) 240-3627',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
login: '+18332403627@expensify.sms',
accountID: 4,
pronouns: undefined,
},
{
displayName: 'Lagertha',
avatar: {
Expand All @@ -198,15 +207,6 @@ describe('ReportUtils', () => {
accountID: 5,
pronouns: 'She/her',
},
{
displayName: 'Ragnar',
login: 'ragnar@vikings.net',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
},
accountID: 1,
pronouns: undefined,
},
]);
});
});
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/SidebarFilterTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ describe('Sidebar', () => {
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1);
expect(displayNames).toHaveLength(1);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Four, Three');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four');
} else {
// Both reports visible
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand Down Expand Up @@ -394,7 +394,7 @@ describe('Sidebar', () => {
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(2);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('One, Two');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Four, Three');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Three, Four');
})

// When report3 becomes unread
Expand Down Expand Up @@ -463,7 +463,7 @@ describe('Sidebar', () => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(2);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Four, Three');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('One, Two');
})
);
Expand Down Expand Up @@ -678,7 +678,7 @@ describe('Sidebar', () => {
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1);
expect(displayNames).toHaveLength(1);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Four, Three');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four');
} else {
// Both reports visible
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand Down
Loading