New Thread Report Empty State, Header, LHN, OpenReport, Buttons for DEV#18522
Conversation
|
🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.14-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
|
Hey @chiragsalian @grgia was doing some testing and found that the "Reply in Thread" button has some unexpected behavior on mobile web. It takes you to the LHN and not the thread report. Wanna tackle this in a follow-up? Screen.Recording.2023-05-19.at.2.08.18.PM.mov |
| // Add the createdReportActionID parameter to the API call | ||
| params.createdReportActionID = optimisticCreatedAction.reportActionID; | ||
| // Add a created action, unless we are creating a thread | ||
| if (parentReportActionID === '0') { |
There was a problem hiding this comment.
@stitesExpensify Can you please explain why we don't create the CREATED report action for threads? A thread is also a new report. We think this is causing a bug #19490.
There was a problem hiding this comment.
I think that you're right and we should always do this. It looks like the back end is creating a CREATED action, so we should do it optimistically too.
| // If the item is a thread within a workspace, we will show the subtitle as the second line instead of in a pill | ||
| const alternativeText = optionItem.isThread && optionItem.subtitle ? optionItem.subtitle : optionItem.alternateText; |
There was a problem hiding this comment.
Is it intentional to show workspace name and room name in thread?
As there's no way to see last thread message in LHN, the issue was reported here - #19447.
This only works in DM chat as subtitle not exist.
So I'd like to confirm if it's fine to use optionItem.alternateText here no matter thread or not.
There was a problem hiding this comment.
| if (isThread(report)) { | ||
| const parentReportAction = ReportActionsUtils.getParentReportAction(report); | ||
| const parentReportActionMessage = lodashGet(parentReportAction, ['message', 0, 'text'], '').replace(/(\r\n|\n|\r)/gm, ' '); | ||
| return parentReportActionMessage || Localize.translateLocal('parentReportAction.deletedMessage'); |
There was a problem hiding this comment.
When the action is an Attachment, the title of the report will be Attachment. But this will be translated so we need to handle that ourselves which is being done in #19517.
| /> | ||
| )} | ||
| </View> | ||
| <View style={[styles.threadDividerLine]} /> |
There was a problem hiding this comment.
Coming from #19443
The divider line overlaps with an unread mark, so we should only show the divider line if the first comment in the thread is not marked as unread.
|
Not a regression but a bug: After adding thread reports we had to revisit the implementation of attachment carousel since it's implemented to only cycle through the report actions without including the report parent action which caused a crash #20214. |
| onPress: (closePopover, {reportAction, reportID}) => { | ||
| Report.navigateToAndOpenChildReport(lodashGet(reportAction, 'childReportID', '0'), reportAction, reportID); | ||
| if (closePopover) { | ||
| hideContextMenu(true, ReportActionComposeFocusManager.focus); |
|
We missed listening to thread parent changes on LHN which prevented LHN update when thread parent is changed. |
| } | ||
| const {html, text} = props.fragment; | ||
|
|
||
| if (props.fragment.isDeletedParentAction) { |
There was a problem hiding this comment.
These lines introduced a regression, as in ReportActionItem.js we added a margin based on the report action's isAttachment property.
|
This case was missed while making the changes here. Issue: Inconsistency: Different grayness levels on parent and child message Reproduction Steps:
|
| displayAsGroup={false} | ||
| isMostRecentIOUReportAction={false} | ||
| shouldDisplayNewMarker={false} | ||
| index={0} |
There was a problem hiding this comment.
This didn't consider parent report action index in thread.
More details about the root cause: #30805 (comment)
| */ | ||
| function navigateToAndOpenChildReport(childReportID = '0', parentReportAction = {}, parentReportID = '0') { | ||
| if (childReportID !== '0') { | ||
| openReport(childReportID); |
There was a problem hiding this comment.
Coming from #43120, this line causes OpenReport API called twice when opening an existing thread.
Details
Add the FE associated with creating and displaying thread reports.
This includes:
Appearance in LHN
Thread Headers
Reply in Thread Button in Context Menus
Thread Report Empty State
No replies:
With Replies:
Threads appearing in Search Bar
Deleting a Parent Message
Fixed Issues
$ #18755 - Report Headers
#18759 - Opening a Report (accessible in DEV only)
#18758 - Adding the button (accessible in DEV only)
Tests
Verify that no errors appear in the JS console
In a DM chat, hover on a chat and click the reply in thread button. Ensure a new thread is created.
In a DM chat, right-click on a chat and click the reply in thread button in the pop-up menu. Ensure a new thread is created.
Visually QA the created thread report to ensure that the LHN, Header, and Empty State display the correct information.
In a Workspace chat, create a thread.
Visually QA the created thread report to ensure that the LHN, Header, and Empty State display the correct information.
<Workspace Name>In a Workspace Room chat, create a thread.
Visually QA the created thread report to ensure that the LHN, Header, and Empty State display the correct information.
<Workspace Name> • #<roomName>Go to a chat that you've already created a thread for and replied to. Click reply in the thread. Ensure that the existing report opens with your existing reply (and not a new one).
Send a message. Now create a thread from that message. Then go back to the original message in the parent report and delete it. Ensure it shows in BOTH parent report and child report as [Deleted Message]
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android