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: regressions related to workspace avatars #41485

Merged
merged 13 commits into from
May 15, 2024
Merged

fix: regressions related to workspace avatars #41485

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
00dd36f
fix: Each workspace avatar in sender list has the same avatar
nkdengineer May 2, 2024
e8f3568
fix: regression 41387
nkdengineer May 2, 2024
89c652b
remove useless code
nkdengineer May 3, 2024
b3e8557
resolve conflict
nkdengineer May 7, 2024
9ca04c6
fix: typo
nkdengineer May 8, 2024
afdc4a0
Add comment to report avatar
nkdengineer May 9, 2024
1a416d2
Fix the avatar of report preview action
nkdengineer May 9, 2024
1ad57da
Merge branch 'main' into fix/41385
nkdengineer May 10, 2024
18c70b4
Merge branch 'main' into fix/41385
nkdengineer May 13, 2024
be0f122
rename accountID prop to avatarID prop
nkdengineer May 13, 2024
ce7f3a4
merge main
nkdengineer May 14, 2024
c911404
resolve conflict
nkdengineer May 14, 2024
e602e57
Update src/pages/ReportAvatar.tsx
nkdengineer May 15, 2024
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
5 changes: 5 additions & 0 deletions src/components/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,9 @@ type MenuItemBaseProps = {

/** Handles what to do when the item is focused */
onFocus?: () => void;

/** Optional account id if it's user avatar or policy id if it's workspace avatar */
accountID?: number | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a prop with name accountID and holding value of policyID is confusing IMO.
We can either have two props - policyID and accountID and use one that's provided - policyID ?? accountID
Or, we can have a prop avatarID that takes either policyID or accountID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think avatarID is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MonilBhavsar I updated with new prop name

};

type MenuItemProps = (IconProps | AvatarProps | NoIcon) & MenuItemBaseProps;
Expand Down Expand Up @@ -334,6 +337,7 @@ function MenuItem(
isPaneMenu = false,
shouldPutLeftPaddingWhenNoIcon = false,
onFocus,
accountID = '',
Copy link
Member

Choose a reason for hiding this comment

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

Can you point to me where in the code this will actually be used? because in this PR your not passing accountID anywhere to MenuItem and I would like understand the flow of props better.

Also please don't default to '' - accountID makes no sense with empty value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks your comment
I missed adding props in MoneyRequestConfirmationList when resolving conflict

}: MenuItemProps,
ref: PressableRef,
) {
Expand Down Expand Up @@ -515,6 +519,7 @@ function MenuItem(
fallbackIcon={fallbackIcon}
name={title}
type={CONST.ICON_TYPE_WORKSPACE}
accountID={accountID}
/>
)}
{iconType === CONST.ICON_TYPE_AVATAR && (
Expand Down
1 change: 1 addition & 0 deletions src/components/UserDetailsTooltip/BaseUserDetailsTooltip/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ function BaseUserDetailsTooltip({accountID, fallbackUserDetails, icon, delegateA
type={icon?.type ?? CONST.ICON_TYPE_AVATAR}
name={icon?.name ?? userLogin}
fallbackIcon={icon?.fallbackIcon}
accountID={icon?.id}
/>
</View>
<Text style={[styles.mt2, styles.textMicroBold, styles.textReactionSenders, styles.textAlignCenter]}>{title}</Text>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReportAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ function ReportAvatar({report = {} as Report, policies, isLoadingApp = true}: Re
}}
isWorkspaceAvatar
maybeIcon
originalFileName={policy?.originalFileName ?? policyName}
originalFileName={policy?.originalFileName ?? policy?.id}
Copy link
Member

Choose a reason for hiding this comment

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

Technically this will work, but the prop name is called originalFileName so passing policy.id will confuse people in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to explain, please check again

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add why we need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        // In the case of the default workspace avatar, we will use the policy id that will be originalFileName prop to get the color of the avatar.

@MonilBhavsar I added a comment.

shouldShowNotFoundPage={!report?.reportID && !isLoadingApp}
isLoading={(!report?.reportID || !policy?.id) && !!isLoadingApp}
/>
Expand Down
1 change: 1 addition & 0 deletions src/pages/iou/request/step/IOURequestStepSendFrom.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function IOURequestStepSendFrom({route, transaction, allPolicies}: IOURequestSte
keyForList: policy.id,
icons: [
{
id: policy.id,
source: policy?.avatarURL ? policy.avatarURL : ReportUtils.getDefaultWorkspaceAvatar(policy.name),
fallbackIcon: Expensicons.FallbackWorkspaceAvatar,
name: policy.name,
Expand Down
Loading