Add support for viewing full screen Group Chat custom avatars #41586

nexarvo · 2024-05-03T14:50:00Z

Details

Fixed Issues

$ #39850
#39760
PROPOSAL: #39850 (comment)

Tests

Group Chat Case:

Go to any group chat
Click on the header
Click on the chat avatar
In the menu click on "View Photo"
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Creating new Group Chat Case:

Click on the '+' button in main tab navigator.
Click on start new chat.
Select participant/s by clicking 'Add to group'.
In the New chat confirm modal, click on the avatar and upload a new avatar.
Now click on the avatar > click on 'View Photo'.
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Group Chat Thread Case:

Go to any group chat
Start a thread
Click on the header of the thread to go to the Details page
Hover over the profile picture. Notice that there is a Pointer Cursor.
Click on the Profile Picture.
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Verify that no errors appear in the JS console

Offline tests

Group Chat Case:

Go to any group chat
Click on the header
Click on the chat avatar
In the menu click on "View Photo"
Verify that full screen modal opens with message "You appear to be offline.".

Creating new Group Chat Case:

Click on the '+' button in main tab navigator.
Click on start new chat.
Select participant/s by clicking 'Add to group'.
In the New chat confirm modal, click on the avatar and upload a new avatar.
Now click on the avatar > click on 'View Photo'.
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Group Chat Thread Case:

Go to any group chat
Start a thread
Click on the header of the thread to go to the Details page
Hover over the profile picture. Notice that there is a Pointer Cursor.
Click on the Profile Picture.
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

QA Steps

Group Chat Case:

Go to any group chat
Click on the header
Click on the chat avatar
In the menu click on "View Photo"
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Creating new Group Chat Case:

Click on the '+' button in main tab navigator.
Click on start new chat.
Select participant/s by clicking 'Add to group'.
In the New chat confirm modal, click on the avatar and upload a new avatar.
Now click on the avatar > click on 'View Photo'.
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Group Chat Thread Case:

Go to any group chat
Start a thread
Click on the header of the thread to go to the Details page
Hover over the profile picture. Notice that there is a Pointer Cursor.
Click on the Profile Picture.
Verify that avatar opens in full screen and we can dismiss the modal by clicking "X".

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-05-25.at.12.09.09.AM.mov

Screen.20Recording.202024-05-25.20at.2012-5.mp4

Screen.20Recording.202024-05-25.20at.2012-6.mp4

Android: mWeb Chrome

Screen.20Recording.202024-05-25.20at.2012-4.mp4

Screen.Recording.2024-05-25.at.12.33.42.AM.mov

Screen.Recording.2024-05-25.at.12.35.51.AM.mov

iOS: Native

Getting build errors.

iOS: mWeb Safari

RPReplay_Final1716579777.MP4

RPReplay_Final1716579821.MP4

RPReplay_Final1716579969.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-05-03.at.8.23.14.PM.mov

Screen.20Recording.202024-05-24.20at.2011-2.mp4

Screen.20Recording.202024-05-24.20at.2011.mp4

MacOS: Desktop

Screen.20Recording.202024-05-25.20at.2012-2.mp4

Screen.20Recording.202024-05-25.20at.2012-3.mp4

Screen.20Recording.202024-05-25.20at.2012.mp4

melvin-bot · 2024-05-03T15:30:10Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

s77rt · 2024-05-04T17:13:42Z

src/pages/ReportAvatar.tsx

            defaultOpen
            source={UserUtils.getFullSizeAvatar(avatarURL, 0)}
            onModalClose={() => {
                Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report?.reportID ?? ''));
            }}
            isWorkspaceAvatar
            maybeIcon
-            originalFileName={policy?.originalFileName ?? policyName}
+            originalFileName={policy?.originalFileName ?? title}


NAB. Can you move this to a separate variable like the rest

Done. Please check

s77rt · 2024-05-04T23:27:17Z

Also complete the checklist

nexarvo · 2024-05-06T14:10:12Z

@s77rt Need to confirm one thing, do we need to add "View Photo" functionality for starting a new group chat modal?

s77rt · 2024-05-06T16:29:44Z

Yes, let's add support for that one too

s77rt · 2024-05-11T12:23:53Z

@nexarvo Any update on this? Let me know if there is something blocking this

nexarvo · 2024-05-13T16:34:11Z

sorry for the delay. I will update and push the code for this by today EOD.

nexarvo · 2024-05-17T10:43:06Z

@s77rt can you please review this commit for this feature. Once approved I will test all the edge cases related to this.

s77rt · 2024-05-17T16:06:04Z

src/pages/ReportAvatar.tsx

+    if(!avatarURL && groupChatDraft?.avatarUri) {
+      avatarURL = `${groupChatDraft.avatarUri}.jpg`;
+    }


This is a hacky solution. What we can do instead is get the file name and pass it as prop originalFileName. This will pass the condition your referenced in AttachmentView

Added originalFileName in this commit.

src/pages/NewChatConfirmPage.tsx

s77rt · 2024-05-20T16:54:22Z

src/pages/ReportAvatar.tsx

+    let avatarURL = policy ? ReportUtils.getWorkspaceAvatar(report) : report?.avatarUrl;
+    let fileName = policy?.originalFileName ?? title;
+
+    if (!avatarURL && groupChatDraft?.avatarUri && groupChatDraft?.originalFileName) {


This fallback logic will not work correctly in cases where we'd expect to see a not found page i.e. we will always fallback to the draft avatar. Can you instead add a query params that determines if we want to use the group chat draft?

s77rt · 2024-05-20T16:57:21Z

src/pages/ReportAvatar.tsx

+    groupChatDraft: {
+        key: ONYXKEYS.NEW_GROUP_CHAT_DRAFT,
+    },


Let's make this use the useOnyx hook instead. It should help avoid loading the data from onyx if we are not going to use it. Related to the point above ^

nexarvo · 2024-05-23T19:29:55Z

@s77rt updated the code to address this and this

s77rt

Fix the conflicts please

s77rt · 2024-05-24T05:07:45Z

src/pages/NewChatConfirmPage.tsx

                    }}
                    size={CONST.AVATAR_SIZE.XLARGE}
                    avatarStyle={styles.avatarXLarge}
-                    shouldDisableViewPhoto


This prop is not removed yet from AvatarWithImagePicker

s77rt · 2024-05-24T05:09:56Z

src/pages/ReportAvatar.tsx

+    if(shouldUseGroupChatDraft) {
+        [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT);
+        avatarURL = groupChatDraft?.avatarUri ?? undefined;
+        fileName = groupChatDraft?.originalFileName ?? undefined;
+    }


Hooks cannot be conditioned.

s77rt · 2024-05-24T05:16:01Z

src/pages/ReportAvatar.tsx

-            originalFileName={policy?.originalFileName ?? policyName}
-            shouldShowNotFoundPage={!report?.reportID && !isLoadingApp}
+            originalFileName={fileName}
+            shouldShowNotFoundPage={!report?.reportID && !groupChatDraft && !isLoadingApp}


Related to the above ^. Since groupChatDraft will pretty much always be set we will not see the not found page in any case. Please update the logic accordingly

If we remove ! groupChatDraft from above condition then for groupChatDraft it always return not found page.

s77rt · 2024-05-24T05:16:44Z

src/ROUTES.ts

@@ -213,7 +213,7 @@ const ROUTES = {
    },
    REPORT_AVATAR: {
        route: 'r/:reportID/avatar',
-        getRoute: (reportID: string) => `r/${reportID}/avatar` as const,
+        getRoute: (reportID: string, newGroupChat: boolean) => `r/${reportID}/avatar?newGroupChat=${newGroupChat}` as const,


Let's not pass the query param if the newGroupChat param is not true

NAB. Change the param name to isNewGroupChat

Thanks. Added conditional to return route based on isNewGroupChat here.

s77rt · 2024-05-24T05:20:44Z

src/libs/Navigation/types.ts

@@ -884,6 +884,7 @@ type AuthScreensParamList = SharedScreensParamList & {
    };
    [SCREENS.REPORT_AVATAR]: {
        reportID: string;
+        newGroupChat: string


This should be boolean. Then in src/libs/Navigation/linkingConfig/config.ts add

parse: { newGroupChat: (newGroupChat: string) => newGroupChat === "true", },

This is done.

s77rt · 2024-05-25T18:14:35Z

src/libs/Navigation/types.ts

@@ -881,7 +881,7 @@ type AuthScreensParamList = SharedScreensParamList & {
    };
    [SCREENS.REPORT_AVATAR]: {
        reportID: string;
-        newGroupChat: string
+        isNewGroupChat: boolean;


Suggested change

isNewGroupChat: boolean;

isNewGroupChat?: boolean;

s77rt · 2024-05-25T18:17:36Z

src/pages/ReportAvatar.tsx

-    let groupChatDraft;
+    let avatarURL;
+    let fileName;
+    const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT);


Suggested change

const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT);

const shouldUseGroupChatDraft = !!route.params.isNewGroupChat;

const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT, {initWithStoredValues: shouldUseGroupChatDraft});

A little optimization, we don't need to get onyx values if we are not going to use it

s77rt · 2024-05-25T18:33:57Z

Bug: The report name (title) is not displayed

s77rt · 2024-05-25T18:35:55Z

Bug: If you try to access the avatar of a non-existing report you will not see the not found view (because groupChatDraft is present)
Solution: Write a conditioned shouldShowNotFoundPage variable

nexarvo · 2024-05-27T10:03:55Z

Bug: The report name (title) is not displayed

I think is from new group chat. Actually we are showing policyName or report.title in the title of the modal. For new group chat, if the user does not enters custom name foe group chat the default name is not added to the GroupChatDraft. We can update the logic to add default chat name to GroupChatDraft as well here, so that it does not show as empty.
thoughts @s77rt ?

nexarvo · 2024-05-27T10:05:38Z

Bug: If you try to access the avatar of a non-existing report you will not see the not found view (because groupChatDraft is present) Solution: Write a conditioned shouldShowNotFoundPage variable

Will fix this today

s77rt · 2024-05-27T10:49:34Z

src/pages/ReportAvatar.tsx

    if (shouldUseGroupChatDraft) {
        avatarURL = groupChatDraft?.avatarUri ?? undefined;
        fileName = groupChatDraft?.originalFileName ?? undefined;
+        title = groupChatDraft?.reportName ?? '';


Use ReportUtils.getReportName

ReportUtils.getReportName takes report as input but in this case we have groupChatDraft which is of type: OnyxEntry<NewGroupChatDraft>.

App/src/libs/ReportUtils.ts

Line 3147 in 3bf6bcf

function getReportName(report: OnyxEntry<Report>, policy: OnyxEntry<Policy> = null): string {

Let's use ReportUtils.getGroupChatName then

nexarvo · 2024-05-29T14:02:40Z

Bug: If you try to access the avatar of a non-existing report you will not see the not found view (because groupChatDraft is present) Solution: Write a conditioned shouldShowNotFoundPage variable

Updated the condition in this commit.

s77rt · 2024-05-29T20:05:22Z

src/pages/ReportAvatar.tsx

    if (shouldUseGroupChatDraft) {
        avatarURL = groupChatDraft?.avatarUri ?? undefined;
        fileName = groupChatDraft?.originalFileName ?? undefined;
+        title = groupChatDraft?.reportName ?? '';


Let's use ReportUtils.getGroupChatName then

s77rt · 2024-05-29T20:08:55Z

src/pages/ReportAvatar.tsx

+        title = policy ? ReportUtils.getPolicyName(report, false, policy) : report?.reportName;
+    }
+
+    const shouldShowNotFoundPage = !isLoadingApp && (shouldUseGroupChatDraft ? !groupChatDraft : !policy && !report?.reportID);


Move to the if condition above and add eslint ignore comment (false positive)

s77rt · 2024-06-05T15:26:05Z

@nexarvo Any updates here?

nexarvo · 2024-06-06T10:57:38Z

@s77rt the code is updated now and ready for review :)

s77rt

I'm unable to test this, all attachments are stuck in loading state, can you try merge main see if that fixes it

s77rt · 2024-06-06T13:31:29Z

src/pages/ReportAvatar.tsx

+        fileName = groupChatDraft?.originalFileName ?? undefined;
+        // When user enters custom group name, it typically stored in groupChatDraft.reportName
+        // If that is null then we will use ReportUtils.getGroupChatName to get the name
+        title = groupChatDraft?.reportName ? groupChatDraft?.reportName : ReportUtils.getGroupChatName(groupChatDraft?.participants.map((x) => x.accountID) ?? []);


Suggested change

title = groupChatDraft?.reportName ? groupChatDraft?.reportName : ReportUtils.getGroupChatName(groupChatDraft?.participants.map((x) => x.accountID) ?? []);

title = groupChatDraft?.reportName || ReportUtils.getGroupChatName(groupChatDraft?.participants.map((participant) => participant.accountID) ?? []);

s77rt · 2024-06-06T13:34:24Z

src/pages/ReportAvatar.tsx

+    } else {
+        avatarURL = policy ? ReportUtils.getWorkspaceAvatar(report) : report?.avatarUrl;
+        // In the case of default workspace avatar, originalFileName prop takes policyID as value to get the color of the avatar
+        fileName = policy ? policy?.originalFileName ?? policy?.id : title;


Why are you adding title as fallback? Also it's undefined at this point

s77rt · 2024-06-06T13:34:54Z

src/pages/ReportAvatar.tsx

+        if (policy) {
+            title = ReportUtils.getPolicyName(report, false, policy);
+        } else {
+            title = report?.reportName ? report.reportName : ReportUtils.getGroupChatName(undefined, false, report?.reportID);
+        }


Suggested change

if (policy) {

title = ReportUtils.getPolicyName(report, false, policy);

} else {

title = report?.reportName ? report.reportName : ReportUtils.getGroupChatName(undefined, false, report?.reportID);

}

title = policy ? ReportUtils.getPolicyName(report, false, policy) : ReportUtils.getReportName(report);

s77rt · 2024-06-06T13:37:16Z

src/pages/ReportAvatar.tsx

+        } else {
+            title = report?.reportName ? report.reportName : ReportUtils.getGroupChatName(undefined, false, report?.reportID);
+        }
+        shouldShowNotFoundPage = !isLoadingApp && !policy && !report?.reportID;


Is there a reason why we are adding the policy check?

enable view photo for group chat

7ce2242

nexarvo marked this pull request as ready for review May 3, 2024 15:30

nexarvo requested a review from a team as a code owner May 3, 2024 15:30

melvin-bot bot removed the request for review from a team May 3, 2024 15:30

melvin-bot bot requested a review from s77rt May 3, 2024 15:30

s77rt reviewed May 4, 2024

View reviewed changes

create variable for filename

b12d3ef

Merge branch 'main' into feat/39850

349ea43

add support for new chat view photo

5b6a7c6

s77rt suggested changes May 17, 2024

View reviewed changes

nexarvo added 2 commits May 20, 2024 14:55

revert removing extension

66d8edc

use file name for new group chat draft

7d122da

s77rt suggested changes May 20, 2024

View reviewed changes

nexarvo added 2 commits May 24, 2024 00:22

add newGroupChat query param for report avatar

91f73c8

clean code

20294f6

s77rt suggested changes May 24, 2024

View reviewed changes

nexarvo added 6 commits May 24, 2024 22:02

Merge branch 'main' into feat/39850

c9993e8

remove shouldDisableViewPhoto param

e4ae8b4

add: conditional for route depending on isNewGroupChat

b26a166

add: parser to convert isNewGroupChat to boolean

1e29cab

update: made isNewGroupChat query param optional

0b0da6c

refactor: clean code

10eade0

nexarvo added 2 commits May 24, 2024 23:49

fix: lint issues

232e787

fix: lint issues

8ff95be

s77rt suggested changes May 25, 2024

View reviewed changes

fix: minor logic issues

284d1f0

s77rt suggested changes May 27, 2024

View reviewed changes

fix: shouldShowNotFoundPage condition

3bf6bcf

Merge branch 'main' into feat/39850

2992240

s77rt suggested changes May 29, 2024

View reviewed changes

nexarvo added 2 commits June 4, 2024 15:15

Merge branch 'main' into feat/39850

a4315b7

fix: handle edge cases for group chat name

f6f582c

fix: report title edge cases

cf3f942

s77rt suggested changes Jun 6, 2024

View reviewed changes

	const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT);
	const shouldUseGroupChatDraft = !!route.params.isNewGroupChat;
	const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT, {initWithStoredValues: shouldUseGroupChatDraft});

	title = groupChatDraft?.reportName ? groupChatDraft?.reportName : ReportUtils.getGroupChatName(groupChatDraft?.participants.map((x) => x.accountID) ?? []);
	title = groupChatDraft?.reportName \|\| ReportUtils.getGroupChatName(groupChatDraft?.participants.map((participant) => participant.accountID) ?? []);

Add support for viewing full screen Group Chat custom avatars #41586

Are you sure you want to change the base?

Add support for viewing full screen Group Chat custom avatars #41586

Conversation

nexarvo commented May 3, 2024 • edited

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented May 4, 2024

nexarvo commented May 6, 2024

s77rt commented May 6, 2024

s77rt commented May 11, 2024

nexarvo commented May 13, 2024

nexarvo commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nexarvo commented May 23, 2024

s77rt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented May 25, 2024 • edited

s77rt commented May 25, 2024

nexarvo commented May 27, 2024

nexarvo commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nexarvo commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Jun 5, 2024

nexarvo commented Jun 6, 2024

s77rt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nexarvo commented May 3, 2024 •

edited

s77rt commented May 25, 2024 •

edited