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.

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

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

Closed
marcaaron opened this issue Apr 15, 2024 · 75 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Apr 15, 2024

As discussed with @shawnborton and @JmillsExpensify we want to update the design of the Report Details page with the following changes:

Result:

Image

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b00ddeb3014a078a
  • Upwork Job ID: 1779985331229118464
  • Last Price Increase: 2024-04-22
  • Automatic offers:
    • DylanDylann | Contributor | 0
    • ZhenjaHorbach | Contributor | 0
Issue OwnerCurrent Issue Owner: @JmillsExpensify
@marcaaron marcaaron self-assigned this Apr 15, 2024
@marcaaron marcaaron added NewFeature Something to build that is a new item. External Added to denote the issue can be worked on by a contributor labels Apr 15, 2024
@melvin-bot melvin-bot bot changed the title [Group Chats] [Polish] Move "Leave" button into a row of the Report Details page [$250] [Group Chats] [Polish] Move "Leave" button into a row of the Report Details page Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01b00ddeb3014a078a

Copy link

melvin-bot bot commented Apr 15, 2024

Triggered auto assignment to @greg-schroeder (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2 labels Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Apr 15, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 15, 2024
@JmillsExpensify
Copy link

Going to assign myself as part of the details revamp design doc.

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 15, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

[Group Chats] [Polish] Move "Leave" button into a row of the Report Details page

What is the root cause of that problem?

New features

What changes do you think we should make in order to solve the problem?

We need remove leave button from here and add share button

confirmText={translate('common.leave')}

            <View style={[styles.flex1]}>
                <Button
                    onPress={() => {
                        Navigation.navigate(ROUTES.SETTINGS_SHARE_CODE);
                    }}
                    icon={Expensicons.QrCode}
                    style={styles.flex1}
                    text={translate('common.share')}
                />
            </View>

And add leave button inside menuItems

const menuItems: ReportDetailsPageMenuItem[] = useMemo(() => {

        // Optionally we can add a button display and update logic for the chat room following this comment https://github.com/Expensify/App/issues/40256#issuecomment-2059803394
        
        if (isGroupChat) {
            items.push({
                key: CONST.REPORT_DETAILS_MENU_ITEM.LEAVE_ROOM,
                translationKey: 'common.leave',
                icon: Expensicons.Exit,
                isAnonymousAction: true,
                action: () => {
                    if (Object.keys(report?.participants ?? {}).length === 1) {
                        setIsLastMemberLeavingGroupModalVisible(true);
                        return;
                    }

                    Report.leaveGroupChat(report.reportID);
                },
            });
        }

And confirm modal

                <ConfirmModal
                    danger
                    title={translate('groupChat.lastMemberTitle')}
                    isVisible={isLastMemberLeavingGroupModalVisible}
                    onConfirm={() => {
                        setIsLastMemberLeavingGroupModalVisible(false);
                        Report.leaveGroupChat(report.reportID);
                    }}
                    onCancel={() => setIsLastMemberLeavingGroupModalVisible(false)}
                    prompt={translate('groupChat.lastMemberWarning')}
                    confirmText={translate('common.leave')}
                    cancelText={translate('common.cancel')}
                />

Optional : As I can see we have additional space between menu items and leave button (We can add additional space for the last element)

What alternative solutions did you explore? (Optional)

NA

@allgandalf
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Refactor the Report details page

What is the root cause of that problem?

Feature Request

What changes do you think we should make in order to solve the problem?

We majorly need to make modifications to 2 files ReportDetailsPage here and ChatDetailsQuickActionsBar here, we need to move the leave and share code button and the related functionality, we also need to put the pin option first in the new design.

The code is pretty straightforward, here is how it will look after the changes:

Dark Mode Light mode
Simulator Screenshot - iPhone 15 Pro - 2024-04-16 at 03 54 20 Simulator Screenshot - iPhone 15 Pro - 2024-04-16 at 03 51 07

@dragnoir
Copy link
Contributor

dragnoir commented Apr 16, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Skeleton loader uses lighter color for page header.

What is the root cause of that problem?

New feature

What changes do you think we should make in order to solve the problem?

1- allow ChatDetailsQuickActionsBar to be displayed with rooms

Update here:

{isGroupChat && <ChatDetailsQuickActionsBar report={report} />}

We need to add isChatRoom to allow display with chat rooms too.

- {isGroupChat && <ChatDetailsQuickActionsBar report={report} />}
+ {(isGroupChat || isChatRoom) && <ChatDetailsQuickActionsBar  report={report}  />}

Result:
image

2- move ChatDetailsQuickActionsBar below the description

we should update here moving the ChatDetailsQuickActionsBar above the description jsx block.

3- remove share code from menu items

remove this and remove the const here

App/src/CONST.ts

Line 1900 in e994e69

SHARE_CODE: 'shareCode',

Result:
image

4- add Leave to the end of menu items

add here

if (!isGroupDMChat) {
  items.push({
    key:  CONST.REPORT_DETAILS_MENU_ITEM.LEAVE_ROOM,
    translationKey:  'common.leave',
    icon:  Expensicons.Exit,
    isAnonymousAction:  false,
    action: () =>  Report.leaveGroupDMChat(report.reportID),
  });
}

Result:
image

5- replace Leave inside ChatDetailsQuickActionsBar with shareCode

update ChatDetailsQuickActionsBar we ned to make the PIN at top, then edit the Button component

Also update the text from Share Code to Share

<Button
  onPress={() => {
    Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS_SHARE_CODE.getRoute(report?.reportID ?? ''));
  }}
  icon={Expensicons.QrCode}
  style={styles.flex1}
  text={translate('common.share')}
/>

Result:
image

6- larger avatar

As described above on the task details, we need to make the avatar larger

<Avatar
source={icons[0].source}
imageStyles={styles.avatarLarge}
size={CONST.AVATAR_SIZE.LARGE}

update from avatarLarge to avatarXLarge

<Avatar
  source={icons[0].source}
  imageStyles={styles.avatarXLarge}
  size={CONST.AVATAR_SIZE.XLARGE}
  name={icons[0].name}
  type={icons[0].type}
  fallbackIcon={icons[0].fallbackIcon}
/>

Result:
image

7- add shouldShowRightIcon to items

We need to make the Leave a "one tap" action, for that we need to pass shouldShowRightIcon: false hereenter link description here
We need to adjust the itms adding shouldShowRightIcon

items.push({
  key:  CONST.REPORT_DETAILS_MENU_ITEM.LEAVE_ROOM,
  translationKey:  'common.leave',
  icon:  Expensicons.Exit,
  isAnonymousAction:  false,
  action: () =>  Report.leaveGroupChat(report.reportID),
+ shouldShowRightIcon:  false,
});

Result:
image

NB: Some tests are required to make sure no regressions.

8- new updates from #40262 (comment)

After the new requirements from the design team #40262 (comment) I am adding moving the room/group description above the PIN/SHARE buttons

Result:
image

9- move Workspace menu item to just Text under the reem name menu item

to fit the final design mockup #40262 (comment) We need to move Workspace menu item to just Text under the reem name menu item.

image

@DylanDylann
Copy link
Contributor

DylanDylann commented Apr 16, 2024

Taking over as C+ role

@ntdiary ntdiary removed their assignment Apr 16, 2024
@DylanDylann
Copy link
Contributor

DylanDylann commented Apr 16, 2024

@marcaaron @JmillsExpensify

{isGroupChat && <ChatDetailsQuickActionsBar report={report} />}

we only display ChatDetailsQuickActionsBar in the report detail If the report is a group chat. Could you help to confirm

  1. Do we also display the ChatDetailsQuickActionsBar for the room report?
  2. whether the avatar should be displayed larger

@marcaaron
Copy link
Contributor Author

@DylanDylann

whether the avatar should be displayed larger

I think we can ignore the avatar for the purposes of this issue. We're focusing on the "Leave" + "Share" actions in this ticket.

Do we also display the ChatDetailsQuickActionsBar for the room report

We did this largely to focus the introduction of this behavior and allow testing it for Group Chats in isolation. But I think ideally we would take this opportunity to standardize the UI for this. What do you think @JmillsExpensify?

Since the "Leave" action is the only one with a behavior specific to Group Chats - we should be able to show this for Rooms as well. But we can investigate as part of this issue to make sure that assumption is correct.

@dragnoir
Copy link
Contributor

Proposal

@DylanDylann proposal updated after the screenshots here #40262 (comment) from the design team

@dannymcclain
Copy link
Contributor

@dragnoir same as the other issue, but posting here for posterity:

image

Figma link

@dragnoir
Copy link
Contributor

Proposal

Updated based on the last design mockup

@DylanDylann
Copy link
Contributor

@dannymcclain It means that we still display ChatDetailsQuickActionsBar on both group and room detail pages.

  • In the group detail page we will display pin and share option
  • In the room detail page we will display leave and pin option

cc @marcaaron @JmillsExpensify

@DylanDylann
Copy link
Contributor

@dragnoir we should confirm requirements clearly before updating the proposal to save effort 😄

@shawnborton
Copy link
Contributor

I guess you can't Join groups or #announce rooms as a heads up.

But @DylanDylann I agree with your "Expect" screenshot, though I think when we have just one button, we use a max-width of 50% of the space or something like that.

So cc @grgia - just want to make sure you have visibility into what's happening on this issue.

@ZhenjaHorbach
Copy link
Contributor

@shawnborton For more understandable

This is staging

Screenshot 2024-05-10 at 11 30 24 ### Expect Screenshot 2024-05-10 at 11 29 40 Is it ok?

cc @marcaaron

@shawnborton For more understandable

This is staging

Screenshot 2024-05-10 at 11 30 24 ### Expect Screenshot 2024-05-10 at 11 29 40 Is it ok?

cc @marcaaron

In fact, in the case of the Report Details page, we do not have cases where we will only have one button displayed
Since in the case of Share button we decided to use it everywhere

And in the case of a pin button, it should also be everywhere (just look at the header report, where the pin button is always present )

@shawnborton
Copy link
Contributor

Okay cool, that makes sense to me.

@JmillsExpensify
Copy link

Ah, right. We do want to update that everywhere. But I guess I'd be curious to know from @JmillsExpensify - how are we managing these group-specific GHs (that have changes we want everywhere) with your Details clean up project?

@shawnborton good question. This one wasn't on my radar, though what we're doing here will definitely affect what we doing across the platform for the "details revamp." @grgia do you have any recommendations for how we handle this? I think what we're doing here is fine, as long as it's future-compatible with the broader changes we are making soon.

@DylanDylann
Copy link
Contributor

DylanDylann commented May 15, 2024

Summary: We always display Pin and Share button in the promoted area in every detail report like this

Screenshot 2024-05-15 at 17 44 05

Please give a thump up if you agree

cc @Expensify/design @grgia @JmillsExpensify @ZhenjaHorbach

@ZhenjaHorbach
Copy link
Contributor

Summary: We always display Pin and Share button in the promoted area in every detail report like this

Screenshot 2024-05-15 at 17 44 05 Please give a thump up if you agree

cc @Expensify/design @grgia @JmillsExpensify @ZhenjaHorbach

And plus additional questions
When we open a thread in groupChat or chatRoom

Should we show a leave button in details page?

@shawnborton
Copy link
Contributor

We should never have "Leave" as a button - if we do have the option to leave a room, it should show as a row at the bottom.

We've got some nice mockups in Figma here for you to see. Just request view access and we'll let you in.

@greg-schroeder
Copy link
Contributor

Hi team - should this issue potentially be solved as part of the work that's being done here?

@marcaaron
Copy link
Contributor Author

marcaaron commented May 16, 2024

Looks kind of unrelated to me tbh. We could combine in into this ticket, but might add more confusion than it avoids. I don't have much of a horse in this race - but feels "outside the process" to combine one ticket into another one if the only thing connecting them is that they are touching the same code.

@greg-schroeder
Copy link
Contributor

Okay thanks Marc!

@JmillsExpensify
Copy link

Working through PR.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 6, 2024
@melvin-bot melvin-bot bot changed the title [$250] [Group Chats] [Polish] Move "Leave" button into a row of the Report Details page [HOLD for payment 2024-06-13] [$250] [Group Chats] [Polish] Move "Leave" button into a row of the Report Details page Jun 6, 2024
Copy link

melvin-bot bot commented Jun 6, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 6, 2024
Copy link

melvin-bot bot commented Jun 6, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.79-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-13. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jun 6, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@DylanDylann] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jun 12, 2024
@DylanDylann
Copy link
Contributor

This issue could be included in #42071, I don't think we need to create a regression test for this issue

Copy link

melvin-bot bot commented Jun 17, 2024

@JmillsExpensify, @marcaaron, @ZhenjaHorbach, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

@JmillsExpensify
Copy link

Yes, and in fact, we have a whole set of regression tests created for a related project, so I agree that we're covered on that front.

@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2024
@JmillsExpensify
Copy link

Payment summary:

@JmillsExpensify
Copy link

Both contributors paid via Upwork. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests

10 participants