Repository navigation
Conversation
|
@eVoloshchak 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] |
|
Some screenshots: Groupe:
Room:
Admin room:
Deprecated group:
Long workspace name:
Long group name:
long room name:
|
|
Archived room:
|
|
@shawnborton you may want to review this! |
| onFocus?: () => void; | ||
|
|
||
| /** The text to display under the title */ | ||
| underTitle?: string; |
There was a problem hiding this comment.
We don't need this as we already have a furtherDetails prop, which does the same
| titleStyle={styles.newKansasLarge} | ||
| shouldCheckActionAllowedOnPress={false} | ||
| description={isGroupChat ? translate('groupConfirmPage.groupName') : translate('newRoomPage.roomName')} | ||
| underTitle={chatRoomSubtitle && !isGroupChat ? `${translate('threads.in')} ${chatRoomSubtitle}` : ''} // "in Workspace X" |
There was a problem hiding this comment.
| underTitle={chatRoomSubtitle && !isGroupChat ? `${translate('threads.in')} ${chatRoomSubtitle}` : ''} // "in Workspace X" | |
| furtherDetails={chatRoomSubtitle && !isGroupChat ? `${translate('threads.in')} ${chatRoomSubtitle}` : ''} |
|
For archived rooms, I think they should look identical to room names that can't be edited, like so: No need to create a separate design style for archived rooms in my opinion. cc @Expensify/design for vis. Otherwise will spin up a test build now. |
This comment has been minimized.
This comment has been minimized.
|
Test build is looking pretty good! |
|
Testing now and reporting a few things: For workspace chats, we should also update the way we display the name: From our mocks: Note that we want to update the workspace room avatar, but I think we can handle that in a different PR. |
Archived rooms don't belong to Workspaces. Do I have just to remove the part where the workspace is? "In....." |
|
Archived rooms are indeed on workspaces, for example: |
for this, updates as we have on groups? Workspace name: |
|
So the thing with workspace rooms is that they have a different title depending if the Submitter or Admin is viewing them. Submitter sees: Admin currently sees: So maybe something like this makes sense? Thoughts @Expensify/design? |
|
I think the "Workspace" label will be double, top and bottom
|
|
Yeah, that's why I am suggesting we just reuse "Room name" - see my mock above |
|
Also I don't think you can rename those, so there wouldn't be an arrow on the right. |
|
Looks good to me Shawn & I agree. And here's a quick mock of an archived room for reference. Look right to you @shawnborton? |
|
Love it, that looks perfect to me. |
|
That looks great from my end as well |
|
I think the idea is to include the small text below the task title, like so: Basically make the details view match the report header view? |
|
Oh yeah cool, sounds like something an external contributor can do - I can spin up an issue to have it fixed |
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
eVoloshchak
left a comment
eVoloshchak
left a comment
There was a problem hiding this comment.
Looks good and tests well!
| const shouldDisableRename = useMemo(() => ReportUtils.shouldDisableRename(report, linkedWorkspace), [report, linkedWorkspace]); | ||
| const isDeprecatedGroupDM = useMemo(() => ReportUtils.isDeprecatedGroupDM(report), [report]); | ||
| const isPolicyExpenseChat = useMemo(() => ReportUtils.isPolicyExpenseChat(report), [report]); | ||
| const shouldShowRoomName = isPolicyExpenseChat || (!ReportUtils.isChatThread(report) && !isTaskReport && !isDeprecatedGroupDM && !isMoneyRequestReport && !isInvoiceRoom); |
There was a problem hiding this comment.
thought: it would be better to put here report types that can have their names modified vs. one thing that can and then several things that can't. Each new report type that should not show the room name will have to join this list to avoid incorrect behavior.
There was a problem hiding this comment.
I updated to
// List the report types that can have their names modified
const canModifyRoomName = !ReportUtils.isChatThread(report) && !isDeprecatedGroupDM && !isPolicyExpenseChat;
// Set shouldShowRoomName based on the new condition
const shouldShowRoomName = canModifyRoomName && !isTaskReport && !isMoneyRequestReport && !isInvoiceRoom;There was a problem hiding this comment.
ah sorry, that doesn't really address my concern. Is it clear to you which report types have modifiable names?
There was a problem hiding this comment.
ah sorry, that doesn't really address my concern. Is it clear to you which report types have modifiable names?
No, it's a bit complicated 😅 I took the same logic from other components.
There was a problem hiding this comment.
Ok, thanks for being transparent - but can we use the open source channel in Slack to confirm what the behavior should be? I am not too sure myself. Or maybe @eVoloshchak can help us since they already approved the PR.
|
@dragnoir whenever this is ready for another review please leave a comment that says "Updated!" so we know to look again. Thanks! |
|
Updated! |
| const shouldDisableRename = useMemo(() => ReportUtils.shouldDisableRename(report, linkedWorkspace), [report, linkedWorkspace]); | ||
| const isDeprecatedGroupDM = useMemo(() => ReportUtils.isDeprecatedGroupDM(report), [report]); | ||
| const parentNavigationSubtitleData = ReportUtils.getParentNavigationSubtitle(report); | ||
| const shouldShowRoomName = |
There was a problem hiding this comment.
We're changing
const shouldShowRoomName = !ReportUtils.isPolicyExpenseChat(report) && !ReportUtils.isChatThread(report) && !ReportUtils.isInvoiceRoom(report);to
const shouldShowRoomName =
!ReportUtils.isChatThread(report) &&
!isDeprecatedGroupDM &&
!isPolicyExpenseChat &&
!isTaskReport &&
!isMoneyRequestReport &&
!isInvoiceRoom &&
!(isMoneyRequestReport || isInvoiceReport || isMoneyRequest);I agree we should confirm this behavior on Slack (and implement it inside of ReportUtils).
@dragnoir, could you start a slack thread to confirm the behavior?
There was a problem hiding this comment.
shouldShowRoomName is taken from here
and I added the new behavior we needed. I don't think it needs to be a utility, because it's used just once on the ReportDetailsPage.
About the last discussion, #40858 (comment) shouldShowRoomName is different from shouldShowRoomName
confirm this behavior
Can you please explain what behavior you mean?
Those new adjustments we already discussed it starting from this comment #40858 (comment) so I think the new behavior of the shouldShowRoomName is already approved.
What do you think?
There was a problem hiding this comment.
@dragnoir, thank you! Missed the summary of changes in #40858 (comment), we should probably include it in the issue details
Could you merge the latest main please?
|
I think we can just close this one out now. |
7 participants
Details
This PR move the Group Chat "Group/Room name" field out of Settings and into Report Details
Fixed Issues
$ #40262
PROPOSAL: #40262 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop