Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.57k stars 2.91k forks source link

[$250] BUG: "Members" section of #admins room doesn't have room name near the top like other rooms #52933

Open jamesdeanexpensify opened 3 days ago

jamesdeanexpensify commented 3 days ago

Details

Coming from this convo, I noticed that when you go into any #admins room and click into the Members section, near the top it doesn't show the room name:

image

In any other room, it does:

image

cc @Expensify/design

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859707716941961146
  • Upwork Job ID: 1859707716941961146
  • Last Price Increase: 2024-11-21
Issue OwnerCurrent Issue Owner: @dukenv0307
melvin-bot[bot] commented 3 days ago

Triggered auto assignment to @OfstadC (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Krishna2323 commented 3 days ago

Proposal


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

BUG: "Members" section of #admins room doesn't have room name near the top like other rooms

What is the root cause of that problem?

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


Pass subtitle={StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report))} to HeaderWithBackButton.

What alternative solutions did you explore? (Optional)

Result

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

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

FitseTLT commented 3 days ago

Edited by proposal-police: This proposal was edited at 2024-11-21 21:15:47 UTC.

Proposal

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

BUG: "Members" section of #admins room doesn't have room name near the top like other rooms

What is the root cause of that problem?

We haven't given the report name as subtitle to HeaderWithBackButton https://github.com/Expensify/App/blob/e9e18573960d492a29b2563237024f0dc3c37f85/src/pages/ReportParticipantsPage.tsx#L368

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

We can give the report name as the subtitle for default rooms

                    subtitle={ReportUtils.isDefaultRoom(report) ? StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report)) : undefined}

If we want we can do the same for isGroupChat too

What alternative solutions did you explore? (Optional)

We can optionally apply it to isAdminRoom only if we want (or even isAnnounceRoom) or may be isGroupChat case too but for other case as iou reports expense reports etc... we surely don't want to show the names

mkzie2 commented 3 days ago

Edited by proposal-police: This proposal was edited at 2024-11-22 11:11:34 UTC.

Proposal

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

"Members" section of #admins room doesn't have room name near the top like other rooms

What is the root cause of that problem?

For the chat room, the member page is in RoomMembersPage and we have a subtitle in the header is the room name here

https://github.com/Expensify/App/blob/686b8b45ca606453e4b8a5f11e2a16e75dd0b15d/src/pages/RoomMembersPage.tsx#L356

For other reports, the member page is in ReportParticipantsPage and we don't pass subtitle prop to HeaderWithBackButton then no report name section is displayed

https://github.com/Expensify/App/blob/686b8b45ca606453e4b8a5f11e2a16e75dd0b15d/src/pages/ReportParticipantsPage.tsx#L367

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

The default room exists for all workspaces so I think we can display the subtitle with the workspace name, it's more useful for the user to know

  1. We can get the additional detail like here and because we only want to display it for the default room of the workspace, we can only get the case in ${chatRoomSubtitle}
    
    const chatRoomSubtitle = useMemo(() => ReportUtils.getChatRoomSubtitle(report), [report, policy]);
    const additionalRoomDetails = `${translate('threads.in')} ${chatRoomSubtitle}`;

2. Pass `subtitle` prop here

subtitle={ReportUtils.isDefaultRoom(report) ? ${StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report))} ${additionalRoomDetails} : ''}


https://github.com/Expensify/App/blob/686b8b45ca606453e4b8a5f11e2a16e75dd0b15d/src/pages/ReportParticipantsPage.tsx#L367

Optional: We can do the same for the chat room member page in `RoomMembersPage`
### What alternative solutions did you explore? (Optional)

We can apply the main solution for only special reports that we want like admin, and announce or for report with `isChatRoom` is `true`.

We can also apply the main solution for other reports like group chat, ... but we will only display the report name in this case

### Result

<img width="1512" alt="Screenshot 2024-11-22 at 12 06 45" src="https://github.com/user-attachments/assets/13ed95f6-29d8-423f-9e3e-10aee4732ab2">

<!---
ATTN: Contributor+

You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we understand the root cause at a minimum even if the solution doesn't directly address it. When we avoid this step we can end up solving the wrong problems entirely or just writing hacks and workarounds.

Instructions for how to review a proposal:

1. Address each contributor proposal one at a time and address each part of the question one at a time e.g. if a solution looks acceptable, but the stated problem is not clear then you should provide feedback and make suggestions to improve each prompt before moving on to the next. Avoid responding to all sections of a proposal at once. Move from one question to the next each time asking the contributor to "Please update your original proposal and tag me again when it's ready for review".

2. Limit excessive conversation and moderate issues to keep them on track. If someone is doing any of the following things please kindly and humbly course-correct them:

- Posting PRs.
- Posting large multi-line diffs (this is basically a PR).
- Skipping any of the required questions.
- Not using the proposal template at all.
- Suggesting that an existing issue is related to the current issue before a problem or root cause has been established.
- Excessively wordy explanations.

3. Choose the first proposal that has a reasonable answer to all the required questions.
-->
mkzie2 commented 3 days ago

@dubielzyk-expensify What do you think about my result above? Display #admin with workspace is more useful for the user to know the workspace of this admin room when visiting the member page.

shawnborton commented 2 days ago

That looks pretty good to me πŸ‘ I think the idea is here we want to be consistent and apply it everywhere, not just certain rooms.

jamesdeanexpensify commented 2 days ago

If it follows the pattern of all the other rooms, I think it would exclude the workspace name, right?

FitseTLT commented 2 days ago

If it follows the pattern of all the other rooms, I think it would exclude the workspace name, right?

Yes

dukenv0307 commented 2 days ago

@shawnborton @jamesdeanexpensify

For the confirmation here

That looks pretty good to me πŸ‘ I think the idea is here we want to be consistent and apply it everywhere, not just certain rooms.

we want to apply this change for other room (group, ...), not just default room (admin, announce)

Another problem: Should we show #admin for all admin rooms? Or #admin in WS?

For the normal room, we're showing the room name (without WS) as a subtitle, and each room has a different name -> that would be fine

Screenshot 2024-11-23 at 00 19 53

But if we do the same for admin rooms, #admin will be shown -> This name is not really useful to me.

shawnborton commented 2 days ago

Another problem: Should we show #admin for all admin rooms? Or #admin in WS?

I think whatever we do, we should try to be consistent. So I think I would opt to just just show the name of the room as it would be shown in the LHN.

dannymcclain commented 2 days ago

I think whatever we do, we should try to be consistent. So I think I would opt to just just show the name of the room as it would be shown in the LHN

Same. I'm down to start with showing it exactly as it shows in the LHN. And if we find that that's not enough, we can adjust further.

dukenv0307 commented 23 hours ago

Let's go with @Krishna2323's proposal since we should try to be consistent.

Screenshot 2024-11-24 at 14 45 32 Screenshot 2024-11-24 at 14 45 53 Screenshot 2024-11-24 at 14 46 31 Screenshot 2024-11-24 at 14 46 44

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 23 hours ago

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

FitseTLT commented 23 hours ago

@jamesdeanexpensify @shawnborton please check out the result the reviewer has shown above. I think there is a misunderstanding the purpose of the original OP was to make default rooms consistent with other user created policy rooms. Wasn't it? not for all iou reports and so on.

dubielzyk-expensify commented 3 hours ago

Agree with the people above. I'm not sure I'd bring it to IOUs, but other than that the above looks good to me. Happy to lean on what others think here if we also think we should have it on IOUs. I don't think it's a big deal if we do, then at least it's consistent across the board