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

src/pages/NewChatConfirmPage.tsx

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.

nexarvo · 2024-07-04T08:13:39Z

@s77rt sorry this is done. Can you please take a look

s77rt · 2024-07-04T11:17:53Z

@nexarvo This is still not working offline. Please double check

Screen.Recording.2024-07-04.at.12.13.01.PM.mov

nexarvo · 2024-07-07T14:58:50Z

@s77rt this is fixed. Can you please check now

s77rt · 2024-07-07T22:49:27Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

s77rt · 2024-07-07T22:10:33Z

src/libs/ReportUtils.ts

@@ -4637,6 +4638,7 @@ function buildOptimisticChatReport(
    parentReportID = '',
    description = '',
    avatarUrl = '',
+    avatarFileName = '',


After adding this new param we need to check all calls to buildOptimisticChatReport the 15th param should be avatarFileName. This is not the case in buildOptimisticWorkspaceChats. (Plaese double check all calls)

@s77rt just checked this. This is fine at all the places

Also conflicts are also resolved

This is not fixed yet

What is not fixed? I have verified in all places wherebuildOptimisticChatReport is used.

In buildOptimisticWorkspaceChats we call buildOptimisticWorkspaceChats and the 15th param is expenseReportId but that param is for avatarFileName and expenseReportId should be the 16th

@s77rt Sorry, for the miss, please have a look.

s77rt · 2024-07-15T10:57:33Z

@nexarvo Any updates here? Also we have conflicts

s77rt · 2024-07-17T20:29:31Z

src/libs/ReportUtils.ts

@@ -4637,6 +4638,7 @@ function buildOptimisticChatReport(
    parentReportID = '',
    description = '',
    avatarUrl = '',
+    avatarFileName = '',


This is not fixed yet

s77rt · 2024-07-17T20:31:03Z

src/libs/actions/Report.ts

+                avatarFileName: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.avatarFileName ?? null,
                avatarUrl: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.avatarUrl ?? null,


Let's not call getAllReports twice and instead define a report const once

marcaaron · 2024-07-19T18:54:01Z

Looks good, thanks for the changes!

OSBotify · 2024-07-19T19:04:44Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-07-19T20:13:00Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.10-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-22T13:35:16Z

🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.0.10-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2024-07-23T08:02:01Z

🚀 Deployed to production by https://github.com/mountiny in version: 9.0.10-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2024-07-25T03:13:36Z

🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

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

add: isNewGroupChat to types after merge main

db65135

fix: view photo when chat created offline(optimistic)

86fb5e7

fix: lint issues

12d1da7

s77rt suggested changes Jul 7, 2024

View reviewed changes

Merge branch 'main' into feat/39850

3695b6b

s77rt suggested changes Jul 17, 2024

View reviewed changes

nexarvo added 3 commits July 18, 2024 01:51

Merge branch 'main' into feat/39850

16ac2ff

making a single getAllReports call and merging main

f2e6e7b

verify parameters of buildOptimisticChatReport

1c3e7db

s77rt approved these changes Jul 19, 2024

View reviewed changes

melvin-bot bot requested a review from marcaaron July 19, 2024 14:43

marcaaron approved these changes Jul 19, 2024

View reviewed changes

marcaaron assigned nexarvo Jul 19, 2024

marcaaron merged commit 78b7ee9 into Expensify:main Jul 19, 2024
15 of 16 checks passed

github-actions bot mentioned this pull request Jul 19, 2024

Deploy Checklist: New Expensify 2024-07-19 #45794

Closed

mountiny mentioned this pull request Jul 21, 2024

[CP Staging] Revert "Add support for viewing full screen Group Chat custom avatars" #45863

Merged

2 tasks

s77rt mentioned this pull request Aug 21, 2024

[$250] Add support for viewing full screen Group Chat custom avatars #39850

Closed

		avatarFileName: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.avatarFileName ?? null,
		avatarUrl: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.avatarUrl ?? null,

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

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

Conversation

nexarvo commented May 3, 2024 • edited Loading

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

nexarvo commented Jul 4, 2024

s77rt commented Jul 4, 2024

nexarvo commented Jul 7, 2024

s77rt commented Jul 7, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

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 Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Jul 19, 2024

OSBotify commented Jul 19, 2024

OSBotify commented Jul 19, 2024

OSBotify commented Jul 22, 2024

OSBotify commented Jul 23, 2024

OSBotify commented Jul 25, 2024

nexarvo commented May 3, 2024 •

edited

Loading

s77rt commented Jul 7, 2024 •

edited

Loading