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

fix: use the same sort order for header display names and avatars in a group chat view #28218

Merged
merged 9 commits into from
Oct 4, 2023
Merged

fix: use the same sort order for header display names and avatars in a group chat view #28218

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a66f4e1
display sorted names for group chats in the header view and LHN
akinwale Sep 26, 2023
85083c2
modify tests to handle display name sorting logic
akinwale Sep 26, 2023
46a7ac0
use displayNamesWithTooltips list to set the sortedDisplayNames value
akinwale Sep 26, 2023
c6a7553
use sorted display names as title in LHN only when the type is chat
akinwale Sep 26, 2023
c43a812
fix merge conflict
akinwale Sep 29, 2023
dab8610
use chain on personalDetailsList
akinwale Oct 2, 2023
80ebbcf
assign full title value based on certain conditions
akinwale Oct 2, 2023
2e3812a
refactor shared code
akinwale Oct 2, 2023
fa70ee0
cleanup fullTitle condition check
akinwale Oct 2, 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
7 changes: 6 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHN.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

lodashGet(optionItem, 'displayNamesWithTooltips', 0) > 1 doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice tip. It works. Just needed to add .length in the second parameter.

? ReportUtils.getDisplayNamesStringFromTooltips(optionItem.displayNamesWithTooltips)
: optionItem.text;

/**
* Show the ReportActionContextMenu modal popover.
*
Expand Down Expand Up @@ -206,7 +211,7 @@ function OptionRowLHN(props) {
<View style={[styles.flexRow, styles.alignItemsCenter, styles.mw100, styles.overflowHidden]}>
<DisplayNames
accessibilityLabel={translate('accessibilityHints.chatUserDisplayNames')}
fullTitle={optionItem.text}
fullTitle={fullTitle}
displayNamesWithTooltips={optionItem.displayNamesWithTooltips}
tooltipEnabled
numberOfLines={1}
Expand Down
62 changes: 44 additions & 18 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,25 +1173,50 @@ function getDisplayNameForParticipant(accountID, shouldUseShortForm = false) {
* @returns {Array}
*/
function getDisplayNamesWithTooltips(personalDetailsList, isMultipleParticipantReport) {
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}`);
}
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,
};
});
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();
}

/**
* 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(', ');
}

/**
Expand Down Expand Up @@ -3721,6 +3746,7 @@ export {
getIcons,
getRoomWelcomeMessage,
getDisplayNamesWithTooltips,
getDisplayNamesStringFromTooltips,
getReportName,
getReport,
getReportIDFromLink,
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ 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;
const title = ReportUtils.getReportName(reportHeaderData);
// 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 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: 'Ragnar Lothbrok',
login: '[email protected]',
displayName: '(833) 240-3627',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
accountID: 1,
login: '[email protected]',
accountID: 4,
pronouns: undefined,
},
{
Expand All @@ -139,15 +139,6 @@ describe('ReportUtils', () => {
accountID: 3,
pronouns: 'She/her',
},
{
displayName: '(833) 240-3627',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
login: '[email protected]',
accountID: 4,
pronouns: undefined,
},
{
displayName: 'Lagertha Lothbrok',
avatar: {
Expand All @@ -157,18 +148,27 @@ describe('ReportUtils', () => {
accountID: 5,
pronouns: 'She/her',
},
{
displayName: 'Ragnar Lothbrok',
login: '[email protected]',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
},
accountID: 1,
pronouns: undefined,
},
]);
});

test('withMultiParticipantReport', () => {
expect(ReportUtils.getDisplayNamesWithTooltips(participantsPersonalDetails, true)).toStrictEqual([
{
displayName: 'Ragnar',
login: '[email protected]',
displayName: '(833) 240-3627',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
accountID: 1,
login: '[email protected]',
accountID: 4,
pronouns: undefined,
},
{
Expand All @@ -189,15 +189,6 @@ describe('ReportUtils', () => {
accountID: 3,
pronouns: 'She/her',
},
{
displayName: '(833) 240-3627',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
login: '[email protected]',
accountID: 4,
pronouns: undefined,
},
{
displayName: 'Lagertha',
avatar: {
Expand All @@ -207,6 +198,15 @@ describe('ReportUtils', () => {
accountID: 5,
pronouns: 'She/her',
},
{
displayName: 'Ragnar',
login: '[email protected]',
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('Three, Four');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Four, Three');
} 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('Three, Four');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Four, Three');
})

// 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('Three, Four');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Four, Three');
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('Three, Four');
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Four, Three');
} else {
// Both reports visible
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand Down
Loading