-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix: use localeCompare to sort in mention list #45324
Changes from 7 commits
8a98242
2f22f26
97ed6e1
8735cb4
07ff80e
d46adcb
a5bea00
3782e22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,8 +3,8 @@ import lodashMapValues from 'lodash/mapValues'; | |||||||||||||||||||||||||||||||||||||
import lodashSortBy from 'lodash/sortBy'; | ||||||||||||||||||||||||||||||||||||||
import type {ForwardedRef} from 'react'; | ||||||||||||||||||||||||||||||||||||||
import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; | ||||||||||||||||||||||||||||||||||||||
import {useOnyx} from 'react-native-onyx'; | ||||||||||||||||||||||||||||||||||||||
import type {OnyxCollection} from 'react-native-onyx'; | ||||||||||||||||||||||||||||||||||||||
import {useOnyx} from 'react-native-onyx'; | ||||||||||||||||||||||||||||||||||||||
import * as Expensicons from '@components/Icon/Expensicons'; | ||||||||||||||||||||||||||||||||||||||
import type {Mention} from '@components/MentionSuggestions'; | ||||||||||||||||||||||||||||||||||||||
import MentionSuggestions from '@components/MentionSuggestions'; | ||||||||||||||||||||||||||||||||||||||
|
@@ -14,6 +14,7 @@ import useCurrentReportID from '@hooks/useCurrentReportID'; | |||||||||||||||||||||||||||||||||||||
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; | ||||||||||||||||||||||||||||||||||||||
import useDebounce from '@hooks/useDebounce'; | ||||||||||||||||||||||||||||||||||||||
import useLocalize from '@hooks/useLocalize'; | ||||||||||||||||||||||||||||||||||||||
import localeCompare from '@libs/LocaleCompare'; | ||||||||||||||||||||||||||||||||||||||
import * as LoginUtils from '@libs/LoginUtils'; | ||||||||||||||||||||||||||||||||||||||
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils'; | ||||||||||||||||||||||||||||||||||||||
import getPolicyEmployeeAccountIDs from '@libs/PolicyEmployeeListUtils'; | ||||||||||||||||||||||||||||||||||||||
|
@@ -56,6 +57,33 @@ type SuggestionPersonalDetailsList = Record< | |||||||||||||||||||||||||||||||||||||
| null | ||||||||||||||||||||||||||||||||||||||
>; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
function getDisplayName(p: PersonalDetails) { | ||||||||||||||||||||||||||||||||||||||
const displayNameFromAccountID = ReportUtils.getDisplayNameForParticipant(p.accountID); | ||||||||||||||||||||||||||||||||||||||
if (!displayNameFromAccountID) { | ||||||||||||||||||||||||||||||||||||||
return p.login?.length ? p.login : ''; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
return displayNameFromAccountID; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||
* the comparison function used to determine which user will come first in the sorted list | ||||||||||||||||||||||||||||||||||||||
* generally, smaller weight means will come first, and if the weight is the same, we'll sort based on displayName/login and accountID | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||
function compareUserInList(first: PersonalDetails & {weight: number}, second: PersonalDetails & {weight: number}) { | ||||||||||||||||||||||||||||||||||||||
// first, we should sort by weight | ||||||||||||||||||||||||||||||||||||||
if (first.weight !== second.weight) { | ||||||||||||||||||||||||||||||||||||||
return first.weight - second.weight; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// next we sort by display name | ||||||||||||||||||||||||||||||||||||||
const displayNameLoginOrder = localeCompare(getDisplayName(first), getDisplayName(second)); | ||||||||||||||||||||||||||||||||||||||
if (displayNameLoginOrder !== 0) { | ||||||||||||||||||||||||||||||||||||||
return displayNameLoginOrder; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
// then fallback on accountID as the final sorting criteria. | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The jsdoc already explains this, and this says what the code is doing, which is obvious from reading the code. Since these comments don't follow our style guide (should use capital letters on the first word), I suggest removing them
Suggested change
|
||||||||||||||||||||||||||||||||||||||
return first.accountID - second.accountID; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
function SuggestionMention( | ||||||||||||||||||||||||||||||||||||||
{value, selection, setSelection, updateComment, isAutoSuggestionPickerLarge, measureParentContainerAndReportCursor, isComposerFocused, isGroupPolicyReport, policyID}: SuggestionProps, | ||||||||||||||||||||||||||||||||||||||
ref: ForwardedRef<SuggestionsRef>, | ||||||||||||||||||||||||||||||||||||||
|
@@ -270,9 +298,11 @@ function SuggestionMention( | |||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||
}) as Array<PersonalDetails & {weight: number}>; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// at this point we are sure that the details are not null, since empty user details have been filtered in the previous step | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||
const sortedPersonalDetails = filteredPersonalDetails.sort(compareUserInList); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
const sortedPersonalDetails = lodashSortBy(filteredPersonalDetails, ['weight', 'displayName', 'login']); | ||||||||||||||||||||||||||||||||||||||
sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => { | ||||||||||||||||||||||||||||||||||||||
suggestions.push({ | ||||||||||||||||||||||||||||||||||||||
text: formatLoginPrivateDomain(PersonalDetailsUtils.getDisplayNameOrDefault(detail), detail?.login), | ||||||||||||||||||||||||||||||||||||||
|
@@ -443,3 +473,5 @@ function SuggestionMention( | |||||||||||||||||||||||||||||||||||||
SuggestionMention.displayName = 'SuggestionMention'; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export default forwardRef(SuggestionMention); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export {compareUserInList}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import {compareUserInList} from '@pages/home/report/ReportActionCompose/SuggestionMention'; | ||
|
||
describe('compareUserInList', () => { | ||
it('Should compare the weight if the weight is different', () => { | ||
const first = {login: 'John Doe', weight: 1, accountID: 1}; | ||
const second = {login: 'Jane Doe', weight: 2, accountID: 2}; | ||
expect(compareUserInList(first, second)).toBe(-1); | ||
dominictb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
|
||
it('Should compare the displayName if the weight is the same', () => { | ||
const first = {login: 'águero', weight: 2, accountID: 3}; | ||
const second = {login: 'Bronn', weight: 2, accountID: 4}; | ||
const third = {login: 'Carol', weight: 2, accountID: 5}; | ||
expect(compareUserInList(first, second)).toBe(-1); | ||
dominictb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expect(compareUserInList(first, third)).toBe(-1); | ||
expect(compareUserInList(second, third)).toBe(-1); | ||
}); | ||
|
||
it('Should compare the accountID if both the weight and displayName are the same', () => { | ||
const first = {login: 'águero', weight: 2, accountID: 6}; | ||
const second = {login: 'aguero', weight: 2, accountID: 7}; | ||
expect(compareUserInList(first, second)).toBe(-1); | ||
dominictb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
}); |
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.