-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01b00ddeb3014a078a |
Triggered auto assignment to @greg-schroeder ( |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary ( |
Going to assign myself as part of the details revamp design doc. |
ProposalPlease 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
And add leave button inside menuItems App/src/pages/ReportDetailsPage.tsx Line 107 in 351f318
And confirm modal
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 |
ProposalPlease 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 The code is pretty straightforward, here is how it will look after the changes:
|
ProposalPlease 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
|
{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} />}
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
Line 1900 in e994e69
SHARE_CODE: 'shareCode', |
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),
});
}
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')}
/>
6- larger avatar
As described above on the task details, we need to make the avatar larger
App/src/components/RoomHeaderAvatars.tsx
Lines 45 to 48 in ed7029b
<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}
/>
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,
});
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
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.
Taking over as C+ role |
App/src/pages/ReportDetailsPage.tsx Line 323 in c75c85f
we only display ChatDetailsQuickActionsBar in the report detail If the report is a group chat. Could you help to confirm
|
I think we can ignore the avatar for the purposes of this issue. We're focusing on the "Leave" + "Share" actions in this ticket.
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. |
Proposal@DylanDylann proposal updated after the screenshots here #40262 (comment) from the design team |
@dragnoir same as the other issue, but posting here for posterity: |
ProposalUpdated based on the last design mockup |
@dannymcclain It means that we still display
|
@dragnoir we should confirm requirements clearly before updating the proposal to save effort 😄 |
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. |
In fact, in the case of the Report Details page, we do not have cases where we will only have one button displayed 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 ) |
Okay cool, that makes sense to me. |
@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. |
Summary: We always display Pin and Share button in the promoted area in every detail report like this Please give a thump up if you agree cc @Expensify/design @grgia @JmillsExpensify @ZhenjaHorbach |
And plus additional questions Should we show a leave button in details page? |
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. |
Hi team - should this issue potentially be solved as part of the work that's being done here? |
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. |
Okay thanks Marc! |
Working through PR. |
|
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:
|
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:
|
This issue could be included in #42071, I don't think we need to create a regression test for this issue |
@JmillsExpensify, @marcaaron, @ZhenjaHorbach, @DylanDylann Huh... This is 4 days overdue. Who can take care of this? |
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. |
Payment summary:
|
Both contributors paid via Upwork. Closing. |
As discussed with @shawnborton and @JmillsExpensify we want to update the design of the Report Details page with the following changes:
ChatDetailsQuickActionsBar
Result:
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @JmillsExpensifyThe text was updated successfully, but these errors were encountered: