Move Leave button into a row of the Report Details page by ZhenjaHorbach · Pull Request #41823 · Expensify/App

ZhenjaHorbach · 2024-05-08T07:24:50Z

Details

Move the "Share" row into the ChatDetailsQuickActionsBar

Move the "Leave" button out of this component and into it's own row here.

Fixed Issues

$ #40256
PROPOSAL: #40256 (comment)

Tests

Open App
Open any GroupChat or Room
Press on header and open details
Verify that Pin and Share buttons are in ActionBar and leave button is in Menu list at the end

Verify that no errors appear in the JS console

Offline tests

Open App
Open any GroupChat or Room
Press on header and open details
Verify that Pin and Share buttons are in ActionBar and leave button is in Menu list at the end

QA Steps

Open App
Open any GroupChat or Room
Press on header and open details
Verify that Pin and Share buttons are in ActionBar and leave button is in Menu list at the end

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android_w5JoDfzq.mp4

Android: mWeb Chrome

android_w5JoDfzq.mp4

iOS: Native

ios_C82AYGGZ.mp4

iOS: mWeb Safari

ios-web_D4Z5n1mS.mp4

MacOS: Chrome / Safari

web_YDYEn9BO.mp4

MacOS: Desktop

desktop_7RFrq62O.mp4

melvin-bot · 2024-05-08T07:24:55Z

@DylanDylann 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]

ZhenjaHorbach · 2024-05-08T07:41:57Z

@DylanDylann
Hello
PR is ready
I'm using !isGroupDMChat for showing ChatDetailsQuickActionsBar to fix regressions related with Share button display

https://github.com/Expensify/App/pull/41823/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4R364

This condition we used before

https://github.com/Expensify/App/pull/41823/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4L123-L132

The only thing
I'm a little unsure whether we should show a Pin button with the same condition?
What do you think ?

DylanDylann · 2024-05-09T05:34:05Z

@ZhenjaHorbach ref: #40256 (comment)

ZhenjaHorbach · 2024-05-09T06:44:21Z

@ZhenjaHorbach ref: #40256 (comment)

In the current changes I use the condition for ChatDetailsQuickActionsBar that we use for Share button at the moment

DylanDylann · 2024-05-13T10:40:16Z

@ZhenjaHorbach We will apply ChatDetailsQuickActionsBar for all reports but we will not display pin/unpin option in all reports. Let's keep the options as before. This is discussed here

DylanDylann · 2024-05-13T10:41:23Z

For example in admin room, we only display Share button

When migrating to use ChatDetailsQuickActionsBar we also need to ensure that ChatDetailsQuickActionsBar only has Share button

ZhenjaHorbach · 2024-05-13T10:42:47Z

@ZhenjaHorbach We will apply ChatDetailsQuickActionsBar for all reports but we will not display pin/unpin option in all reports. Let's keep options as before

I was just writing a comment about this )
I can't think of a case why we shouldn't display pin buttons everywhere
Because just look at the header report, where the pin button is always present

In fact, it's a little confusing this discrepancy between the header and details page

And during this comment we want use pin everywhere where we can do it
#40256 (comment)

ZhenjaHorbach · 2024-05-13T11:23:32Z

@DylanDylann
https://github.com/Expensify/App/pull/41823/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4L123-L131

@ZhenjaHorbach We will apply ChatDetailsQuickActionsBar for all reports but we will not display pin/unpin option in all reports. Let's keep the options as before. This is discussed here

@DylanDylann

And if I understand you correctly, you mean to show the pin button only for isGroupChat || isChatRoom ?
Like here
https://github.com/Expensify/App/pull/40979/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4R363

DylanDylann · 2024-05-15T10:48:54Z

@ZhenjaHorbach Let's wait for the final decision from the design team before moving forward with the PR. Thanks

ZhenjaHorbach · 2024-05-15T10:50:39Z

@ZhenjaHorbach Let's wait for the final decision from the design team before moving forward with the PR. Thanks

I just fixed the conflicts)
But agree, let's wait for the final decision

DylanDylann · 2024-05-16T09:15:45Z

src/pages/ReportDetailsPage.tsx

                        </OfflineWithFeedback>
                    )}
-                    {isGroupChat && <ChatDetailsQuickActionsBar report={report} />}
+                    <ChatDetailsQuickActionsBar report={report} />


@ZhenjaHorbach From my viewpoint, we should update like this

Suggested change

<ChatDetailsQuickActionsBar report={report} />

{!isGroupDMChat && <ChatDetailsQuickActionsBar report={report} />}

To make sure the logic to display the share button is same like before

I will update this line
If something happens, I'll revert the last commit
If we decide to use it everywhere

DylanDylann · 2024-05-16T09:18:33Z

src/pages/ReportDetailsPage.tsx


    const isSelfDM = useMemo(() => ReportUtils.isSelfDM(report), [report]);

+    const shouldShowReportDescription = isChatRoom && (canEditReportDescription || report.description !== '');


Revert the position of this variable to keep the change is clean

DylanDylann · 2024-05-16T09:27:58Z

src/pages/ReportDetailsPage.tsx

+                translationKey: 'common.leave',
+                icon: Expensicons.Exit,
+                isAnonymousAction: true,
+                shouldShowRightIcon: false,


Should add shouldShowRightIcon: true to other item

Sorry
I probably didn't fully understand you
But we only have one place where we don't need an icon
For other cases I have added a condition here

https://github.com/Expensify/App/pull/41823/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4R369

yeah, the current change work well, but I think add shouldShowRightIcon: true to other item will be clearer

Ahhhh
I understand )
No problem
I'll update now

DylanDylann · 2024-05-16T09:37:41Z

@ZhenjaHorbach Minor comment and please merge main again. Thanks

DylanDylann · 2024-05-16T09:38:45Z

src/pages/ReportDetailsPage.tsx

            });
        }

+        if ((isGroupChat || (isChatRoom && ReportUtils.canLeaveChat(report, policy ?? null))) && !isThread) {


Agree with you at this point && !isThread if it is thread we should display leave row

Agree, but sort of out of scope for the original ticket maybe. I believe there is a separate project to make this change.

…ails-page-update

DylanDylann · 2024-05-16T13:48:21Z

@ZhenjaHorbach please check TS error

ZhenjaHorbach · 2024-05-16T13:50:03Z

@ZhenjaHorbach please check TS error

This is normal )
This is a problem in main

I will update the branch when a fix will be in main

…ails-page-update

ZhenjaHorbach · 2024-05-16T18:40:17Z

@DylanDylann
no more TS errors )

marcaaron · 2024-05-28T21:07:27Z

src/pages/ReportDetailsPage.tsx

                        </OfflineWithFeedback>
                    )}
-                    {isGroupChat && <ChatDetailsQuickActionsBar report={report} />}
+                    {!isGroupDMChat && <ChatDetailsQuickActionsBar report={report} />}


We can pin a Group DM chat, but not share it. They are getting deprecated soon. But, I think this is more or less incorrect logic and we should be able to remove the !isGroupDMChat.

marcaaron · 2024-05-28T21:09:47Z

src/pages/ReportDetailsPage.tsx

            });
        }

+        if ((isGroupChat || (isChatRoom && ReportUtils.canLeaveChat(report, policy ?? null))) && !isThread) {


Agree, but sort of out of scope for the original ticket maybe. I believe there is a separate project to make this change.

marcaaron · 2024-05-28T21:14:01Z

@ZhenjaHorbach found one thing if you can fix it - otherwise, looks great.

Thanks for your patience both @DylanDylann and @ZhenjaHorbach as we had temporarily halted merging of PRs, but now resumed. 🙇

…ails-page-update

ZhenjaHorbach · 2024-05-28T21:45:08Z

Fix comments

@marcaaron

I updated the branch and fixed comments
f6202cc

marcaaron · 2024-05-28T22:55:28Z

src/components/ChatDetailsQuickActionsBar.tsx

+                    icon={Expensicons.QrCode}
                    style={styles.flex1}
-                    text={isPinned ? translate('common.unPin') : translate('common.pin')}
+                    text={translate('common.share')}


well, we need to add back the if (!isGroupDMChat) to this part now... I thought that was clear from my last comment sorry if not.

Oh, sorry
Perhaps I misunderstood
But I fixed it
And I updated the styles for one button inside ActionsBar following this comment

#40256 (comment)

Would you mind posting a screen shot of this change? I want to make sure it's what @shawnborton expects. Thanks!

Something like this if we have one button

I think we're working on a component for this that has all of that baked in. @grgia does that sound right?

Check this PR here: #41972

Just want to make sure we aren't duplicating work.

@shawnborton thanks for noticing! It looks like this PR will conflict with #41972

cc @grgia

Thanks for the heads up @shawnborton

I think we're good here though. We just need to fix some conflicts now @ZhenjaHorbach. I've instructed @cdOut to pause the next PR they mentioned since we don't really need it.

@grgia might want to do a quick scan to make sure there aren't any other improvements with open issues that snuck into that design doc, but don't think there are a ton. They should all be in the #vip-split project and categorized by Group Chats.

I updated the PR and made changes to use the new implementation for ActionBar

Although we have TS issue in main
#42798

…ails-page-update

marcaaron

Just had one more small request. Rest of the code looks good, but don't think we need isHidden

marcaaron · 2024-05-30T20:57:39Z

src/components/HeaderWithBackButton/types.ts

    onSelected: () => void;
+
+    /** Whether the item is hidden */
+    isHidden?: boolean;


Why do we have an isHidden prop vs. just not including it in the items passed to the component?

Actually good idea
Thanks for the review
Done )

marcaaron · 2024-05-30T20:59:10Z

src/components/PromotedActionsBar.tsx

+            {promotedActions.map(({key, onSelected, ...props}) => {
+                if (props.isHidden) {
+                    return null;
+                }


Let's just exclude it if it should be hidden? Then this can be removed.

marcaaron · 2024-05-30T21:02:00Z

src/libs/HeaderUtils.ts

 }

+function getShareMenuItem(report: OnyxReport, participants: number[]): ThreeDotsMenuItem {
+    const isGroupDMChat = ReportUtils.isDM(report) && participants.length > 1;


Check for isGroupDMChat when we are deciding whether to add the share menu item or not. Then delete this method as it is only used in one place.

…ails-page-update

marcaaron · 2024-05-30T22:08:42Z

src/pages/ReportDetailsPage.tsx

-                            shouldShowLeaveButton
-                        />
-                    )}
+                    <PromotedActionsBar promotedActions={[PromotedActions.pin(report), ...(isGroupDMChat ? [] : [PromotedActions.share(report)])]} />


NAB but I think it is slightly over engineered to have "pin" and "share" functions that return an object. No need for all these different methods IMO. Just use the objects.

OSBotify · 2024-06-06T12:33:48Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.79-11 🚀

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

situchan · 2024-06-13T19:11:45Z

src/pages/ReportDetailsPage.tsx

+                key: CONST.REPORT_DETAILS_MENU_ITEM.LEAVE_ROOM,
+                translationKey: 'common.leave',
+                icon: Expensicons.Exit,
+                isAnonymousAction: true,


Coming from #43404:
@ZhenjaHorbach was there any reason for setting this value to true?

Hmmm
Actually good question
But I don't think it should be true

Thanks for confirmation

Move Leave button into a row of the Report Details page

f245c2f

ZhenjaHorbach requested a review from a team as a code owner May 8, 2024 07:24

melvin-bot bot requested review from DylanDylann and removed request for a team May 8, 2024 07:24

Remove condition for ChatDetailsQuickActionsBar

43334c4

Update branch and fix conflicts

746ad28

ZhenjaHorbach added 3 commits May 15, 2024 13:03

Update condition for leave modal

ca4e3ad

Update dependency for menuItems

d1814c1

Update conditions for leave button

7c630c8

DylanDylann reviewed May 16, 2024

View reviewed changes

ZhenjaHorbach added 3 commits May 16, 2024 12:13

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

0aa0fb9

…ails-page-update

Fix comments

564b82f

Disable ActionsBar for isGroupDMChat

6c53db0

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

8a9c211

…ails-page-update

marcaaron requested changes May 28, 2024

View reviewed changes

ZhenjaHorbach added 6 commits May 28, 2024 23:18

Fix comments

f6202cc

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

6bdf55c

…ails-page-update

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

37e90c7

…ails-page-update

Fix ts issue

7d6fe50

Fix eslint issue

1c8f9d3

Move isPolicyExpenseChat

2436d71

marcaaron requested changes May 28, 2024

View reviewed changes

Add condition for share button

ec49893

marcaaron mentioned this pull request May 29, 2024

feat: add PromotedActionsBar #41972

Merged

50 tasks

ZhenjaHorbach added 5 commits May 29, 2024 22:34

Fix conflicts and update branch

bf1dac5

Add comment for isHidden

0b0126e

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

ab5a203

…ails-page-update

Refactor types for ActionBar

25a6964

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

d09d4d7

…ails-page-update

DylanDylann approved these changes May 30, 2024

View reviewed changes

melvin-bot bot requested a review from marcaaron May 30, 2024 08:04

marcaaron requested changes May 30, 2024

View reviewed changes

ZhenjaHorbach added 2 commits May 30, 2024 23:39

Fix comments

19c22bf

Merge branch 'main' into move-leave-button-into-row-of-the-report-det…

9f957f5

…ails-page-update

marcaaron approved these changes May 30, 2024

View reviewed changes

marcaaron merged commit ec495a3 into Expensify:main May 30, 2024

github-actions bot mentioned this pull request Jun 4, 2024

Deploy Checklist: New Expensify 2024-06-04 #43037

Closed

bernhardoj mentioned this pull request Jun 5, 2024

[HOLD for payment 2024-06-28] [HOLD for payment 2024-06-24] [$250] Group chat thread - Inconsistency when leaving thread via 3-dot menu and group details page #43126

Closed

6 tasks

melvin-bot bot mentioned this pull request Jun 6, 2024

[HOLD for payment 2024-06-13] [$250] [Group Chats] [Polish] Move "Leave" button into a row of the Report Details page #40256

Closed

situchan reviewed Jun 13, 2024

View reviewed changes

situchan mentioned this pull request Jun 13, 2024

[HOLD for payment 2024-07-24] [$250] Public room - User is able to leave public room when user is not logged in #43404

Closed

6 tasks

	<ChatDetailsQuickActionsBar report={report} />
	{!isGroupDMChat && <ChatDetailsQuickActionsBar report={report} />}


		const isSelfDM = useMemo(() => ReportUtils.isSelfDM(report), [report]);

		const shouldShowReportDescription = isChatRoom && (canEditReportDescription \|\| report.description !== '');

Conversation

ZhenjaHorbach commented May 8, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented May 8, 2024

Uh oh!

ZhenjaHorbach commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DylanDylann commented May 9, 2024

Uh oh!

ZhenjaHorbach commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DylanDylann commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DylanDylann commented May 13, 2024

Uh oh!

ZhenjaHorbach commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZhenjaHorbach commented May 13, 2024

Uh oh!

DylanDylann commented May 15, 2024

Uh oh!

ZhenjaHorbach commented May 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DylanDylann commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DylanDylann commented May 16, 2024

Uh oh!

ZhenjaHorbach commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZhenjaHorbach commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented May 28, 2024

Uh oh!

ZhenjaHorbach commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhenjaHorbach May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhenjaHorbach commented May 8, 2024 •

edited

Loading

ZhenjaHorbach commented May 9, 2024 •

edited

Loading

DylanDylann commented May 13, 2024 •

edited

Loading

ZhenjaHorbach commented May 13, 2024 •

edited

Loading

ZhenjaHorbach commented May 16, 2024 •

edited

Loading

ZhenjaHorbach commented May 28, 2024 •

edited

Loading

ZhenjaHorbach May 29, 2024 •

edited

Loading