-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[Avatars] Fix demoted deploy blockers (cont.) #68120
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
Changes from all commits
c48f5a6
d86849e
19af696
90491ab
adce8c9
49ebfc1
4561336
c7512ee
c897f23
ecd25e1
4789acf
f542ddd
c225aeb
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 |
|---|---|---|
|
|
@@ -22,18 +22,20 @@ import ReportActionAvatars from './ReportActionAvatars'; | |
| import SelectCircle from './SelectCircle'; | ||
| import Text from './Text'; | ||
|
|
||
| type OptionDataWithOptionalReportID = Omit<OptionData, 'reportID'> & {reportID?: string}; | ||
|
|
||
| type OptionRowProps = { | ||
| /** Style for hovered state */ | ||
| hoverStyle?: StyleProp<ViewStyle>; | ||
|
|
||
| /** Option to allow the user to choose from can be type 'report' or 'user' */ | ||
| option: OptionData; | ||
| option: OptionDataWithOptionalReportID; | ||
|
|
||
| /** Whether this option is currently in focus so we can modify its style */ | ||
| optionIsFocused?: boolean; | ||
|
|
||
| /** A function that is called when an option is selected. Selected option is passed as a param */ | ||
| onSelectRow?: (option: OptionData, refElement: View | HTMLDivElement | null) => void | Promise<void>; | ||
| onSelectRow?: (option: OptionDataWithOptionalReportID, refElement: View | HTMLDivElement | null) => void | Promise<void>; | ||
|
|
||
| /** Whether we should show the selected state */ | ||
| showSelectedState?: boolean; | ||
|
|
@@ -45,7 +47,7 @@ type OptionRowProps = { | |
| selectedStateButtonText?: string; | ||
|
|
||
| /** Callback to fire when the multiple selector (checkbox or button) is clicked */ | ||
| onSelectedStatePressed?: (option: OptionData) => void; | ||
| onSelectedStatePressed?: (option: OptionDataWithOptionalReportID) => void; | ||
|
|
||
| /** Whether we highlight selected option */ | ||
| highlightSelected?: boolean; | ||
|
|
@@ -148,12 +150,19 @@ function OptionRow({ | |
| const firstIcon = option?.icons?.at(0); | ||
|
|
||
| // We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade. | ||
| const displayNamesWithTooltips = getDisplayNamesWithTooltips((option.participantsList ?? (option.accountID ? [option] : [])).slice(0, 10), shouldUseShortFormInTooltip, localeCompare); | ||
| const displayNamesWithTooltips = getDisplayNamesWithTooltips( | ||
| (option.participantsList ?? (option.accountID ? [option as OptionData] : [])).slice(0, 10), | ||
| shouldUseShortFormInTooltip, | ||
| localeCompare, | ||
| ); | ||
| let subscriptColor = theme.appBG; | ||
| if (optionIsFocused) { | ||
| subscriptColor = focusedBackgroundColor; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const reportID = (option.iouReportID ?? option.reportID) || undefined; | ||
|
Contributor
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. Is this for handling the cases where the report ID is empty string?
Contributor
Author
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. Exactly. That would lead to fetch the whole report collection |
||
|
|
||
| return ( | ||
| <Hoverable> | ||
| {(hovered) => ( | ||
|
|
@@ -209,10 +218,11 @@ function OptionRow({ | |
| {!!option.icons?.length && !!firstIcon && ( | ||
| <ReportActionAvatars | ||
| subscriptAvatarBorderColor={hovered && !optionIsFocused ? hoveredBackgroundColor : subscriptColor} | ||
| reportID={option.iouReportID ?? option.reportID} | ||
| reportID={reportID} | ||
| accountIDs={!reportID && option.accountID ? [option.accountID] : []} | ||
| size={CONST.AVATAR_SIZE.DEFAULT} | ||
| secondaryAvatarContainerStyle={[StyleUtils.getBackgroundAndBorderStyle(hovered && !optionIsFocused ? hoveredBackgroundColor : subscriptColor)]} | ||
| shouldShowTooltip={showTitleTooltip && shouldOptionShowTooltip(option)} | ||
| shouldShowTooltip={showTitleTooltip && shouldOptionShowTooltip(option as OptionData)} | ||
| /> | ||
| )} | ||
| <View style={contentContainerStyles}> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,10 @@ function useReportActionAvatars({ | |
| action = getReportAction(reportChatReport?.reportID, chatReport.parentReportActionID); | ||
| } | ||
|
|
||
| const [actionChildReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${action?.childReportID}`, {canBeMissing: true}); | ||
|
|
||
| const isAReportPreviewAction = action?.actionName === CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW; | ||
|
|
||
| const isReportArchived = useReportIsArchived(iouReport?.reportID); | ||
|
|
||
| const reportPreviewSenderID = useReportPreviewSenderID({ | ||
|
|
@@ -70,19 +74,26 @@ function useReportActionAvatars({ | |
| chatReport, | ||
| }); | ||
|
|
||
| const policyID = passedPolicyID ?? (chatReport?.policyID === CONST.POLICY.ID_FAKE || !chatReport?.policyID ? (iouReport?.policyID ?? chatReport?.policyID) : chatReport?.policyID); | ||
| const reportPolicyID = iouReport?.policyID ?? chatReport?.policyID; | ||
| const chatReportPolicyIDExists = chatReport?.policyID === CONST.POLICY.ID_FAKE || !chatReport?.policyID; | ||
| const changedPolicyID = actionChildReport?.policyID ?? iouReport?.policyID; | ||
| const shouldUseChangedPolicyID = !!changedPolicyID && changedPolicyID !== chatReport?.policyID; | ||
|
Contributor
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.
|
||
| const retrievedPolicyID = chatReportPolicyIDExists ? reportPolicyID : chatReport?.policyID; | ||
|
|
||
| const policyID = shouldUseChangedPolicyID ? changedPolicyID : (passedPolicyID ?? retrievedPolicyID); | ||
| const policy = usePolicy(policyID); | ||
|
|
||
| const invoiceReceiverPolicyID = chatReport?.invoiceReceiver && 'policyID' in chatReport.invoiceReceiver ? chatReport.invoiceReceiver.policyID : undefined; | ||
| const invoiceReceiverPolicy = usePolicy(invoiceReceiverPolicyID); | ||
|
|
||
| const {chatReportIDAdmins, chatReportIDAnnounce, workspaceAccountID} = policy ?? {}; | ||
| const [policyChatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${chatReportIDAnnounce ?? chatReportIDAdmins}`, {canBeMissing: true}); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const [policyChatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${chatReportIDAnnounce || chatReportIDAdmins}`, {canBeMissing: true}); | ||
|
Contributor
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. Is
Contributor
Author
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. Yep, don't know why but it happens rather often. Screen.Recording.2025-08-12.at.11.24.00.mov |
||
|
|
||
| const delegatePersonalDetails = action?.delegateAccountID ? personalDetails?.[action?.delegateAccountID] : undefined; | ||
| const actorAccountID = getReportActionActorAccountID(action, iouReport, chatReport, delegatePersonalDetails); | ||
|
|
||
| const isAReportPreviewAction = action?.actionName === CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW; | ||
| const isAInvoiceReport = isInvoiceReport(iouReport ?? null); | ||
|
|
||
| const shouldUseActorAccountID = isAInvoiceReport && !isAReportPreviewAction; | ||
|
|
@@ -267,6 +278,13 @@ function useReportActionAvatars({ | |
|
|
||
| let avatars = [primaryAvatar, secondaryAvatar]; | ||
|
|
||
| const isUserWithWorkspaceAvatar = | ||
| avatarType === CONST.REPORT_ACTION_AVATARS.TYPE.SUBSCRIPT && avatars.at(0)?.type === CONST.ICON_TYPE_AVATAR && avatars.at(1)?.type === CONST.ICON_TYPE_WORKSPACE; | ||
| const isWorkspaceWithUserAvatar = | ||
| avatars.at(0)?.type === CONST.ICON_TYPE_WORKSPACE && avatars.at(1)?.type === CONST.ICON_TYPE_AVATAR && avatarType === CONST.REPORT_ACTION_AVATARS.TYPE.MULTIPLE; | ||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||
| const wasReportPreviewMovedToDifferentPolicy = shouldUseChangedPolicyID && isAReportPreviewAction; | ||
|
|
||
| if (shouldUseInvoiceExpenseIcons) { | ||
| avatars = getIconsWithDefaults(invoiceReport); | ||
| } else if (shouldUseMappedAccountIDs) { | ||
|
|
@@ -278,14 +296,13 @@ function useReportActionAvatars({ | |
| if (avatars.every(({type}) => type === CONST.ICON_TYPE_WORKSPACE)) { | ||
| avatarType = isAReportPreviewAction ? CONST.REPORT_ACTION_AVATARS.TYPE.MULTIPLE : CONST.REPORT_ACTION_AVATARS.TYPE.SUBSCRIPT; | ||
| // But if it is a report preview between workspace and another user it should never be displayed as a multiple avatar | ||
| } else if ( | ||
| avatars.at(0)?.type === CONST.ICON_TYPE_WORKSPACE && | ||
| avatars.at(1)?.type === CONST.ICON_TYPE_AVATAR && | ||
| avatarType === CONST.REPORT_ACTION_AVATARS.TYPE.MULTIPLE && | ||
| isAReportPreviewAction | ||
| ) { | ||
| } else if (isWorkspaceWithUserAvatar && isAReportPreviewAction) { | ||
| avatarType = CONST.REPORT_ACTION_AVATARS.TYPE.SUBSCRIPT; | ||
| } | ||
| } else if (isUserWithWorkspaceAvatar && wasReportPreviewMovedToDifferentPolicy) { | ||
| const policyChatReportIcon = {...getWorkspaceIcon(policyChatReport, policy), id: policyID, name: policy?.name}; | ||
| const [firstAvatar] = avatars; | ||
| avatars = [firstAvatar, policyChatReportIcon]; | ||
| } | ||
|
|
||
| return { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,10 @@ function UserListItem<TItem extends ListItem>({ | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const itemAccountID = Number(item.accountID || item.icons?.at(1)?.id) || 0; | ||
|
|
||
| const isThereOnlyWorkspaceIcon = item.icons?.length === 1 && item.icons?.at(0)?.type === CONST.ICON_TYPE_WORKSPACE; | ||
| const shouldUseIconPolicyID = !item.reportID && !item.accountID && !item.policyID; | ||
| const policyID = isThereOnlyWorkspaceIcon && shouldUseIconPolicyID ? String(item.icons?.at(0)?.id) : item.policyID; | ||
|
Contributor
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. Why is item.policyID returning the wrong policy in the first place?
Contributor
Author
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. |
||
|
|
||
| return ( | ||
| <BaseListItem | ||
| item={item} | ||
|
|
@@ -111,7 +115,7 @@ function UserListItem<TItem extends ListItem>({ | |
| </View> | ||
| </PressableWithFeedback> | ||
| )} | ||
| {(!!reportExists || !!itemAccountID || !!item.policyID) && ( | ||
| {(!!reportExists || !!itemAccountID || !!policyID) && ( | ||
| <ReportActionAvatars | ||
| subscriptAvatarBorderColor={hovered && !isFocused ? hoveredBackgroundColor : subscriptAvatarBorderColor} | ||
| shouldShowTooltip={showTooltip} | ||
|
|
@@ -123,7 +127,7 @@ function UserListItem<TItem extends ListItem>({ | |
| reportID={reportExists ? item.reportID : undefined} | ||
| /* eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing */ | ||
| accountIDs={!reportExists && !!itemAccountID ? [itemAccountID] : []} | ||
| policyID={!reportExists && !itemAccountID ? item.policyID : undefined} | ||
| policyID={!reportExists && !!policyID ? policyID : undefined} | ||
| singleAvatarContainerStyle={[styles.actionAvatar, styles.mr3]} | ||
| fallbackDisplayName={item.text ?? item.alternateText ?? undefined} | ||
| /> | ||
|
|
||

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.
Wondering if we can avoid this cast here. Does TS complain if we add
OptionDataWithOptionalReportIDtype too to the argument ingetDisplayNamesWithTooltipsandshouldOptionShowTooltip?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.
Yep, that's why I casted it. These functions doesn't use reportID at all but didn't want to change parameter type for this one case.