Keep chats with draft comments in the LHN#781
Conversation
Gonals
changed the title
Gonals
changed the title
| unreadActionCount: PropTypes.number, | ||
| })), | ||
|
|
||
| // List of draft comments. We don't know the shape, since the keys include the report numbers |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // comments (but are not the open one), and the one matching the URL | ||
| // eslint-disable-next-line max-len | ||
| const reportsToDisplay = _.filter(sortedReports, report => (report.isPinned || (report.unreadActionCount > 0) || report.reportID === reportIDInUrl)); | ||
| const reportsToDisplay = _.filter(sortedReports, report => (report.isPinned || (report.unreadActionCount > 0) || report.reportID === reportIDInUrl || (report.reportID !== reportIDInUrl && get(props.comments, `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${report.reportID}`, '').length > 0))); |
There was a problem hiding this comment.
This filter is becoming pretty long. Can you please remove the eslint-disable and then split this filter onto multiple lines so it's nice and readable?
| reportID: report.reportID, | ||
| isUnread: report.unreadActionCount > 0, | ||
|
|
||
| // eslint-disable-next-line max-len |
There was a problem hiding this comment.
Same here, remove this and shorten the line instead. It looks like get(props.comments could be DRYed up somehow since I see it used elsewhere.
nickmurray47
left a comment
nickmurray47
left a comment
There was a problem hiding this comment.
Looks good and tests well! A few small changes to make it seems.
|
Comments addressed! |
|
Can we see how this looks with super long emails/group names? |
| 'desc', | ||
| 'asc' | ||
| ]); | ||
| const hasComment = reportID => get(props.comments, `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`, '').length > 0; |
There was a problem hiding this comment.
This is still having a lint warning for the line length. I have a few suggestions:
- Make it into a multi-line function like this:
function hasComment(reportID) {
const reportComments = get(props.comments, `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`, '');
return reportComments.length > 1;
}
- Define it outside the component and pass both
reportIDandprops.commentsas arguments
|
|
It looks like the pencil icon and the text are not vertically aligned/centered. Does it look that way to you too? |
| 'desc', | ||
| 'asc' | ||
| ]); | ||
| function hasComment(reportID) { |
There was a problem hiding this comment.
Still missing method docs :D
There was a problem hiding this comment.
Where? They are there!
There was a problem hiding this comment.
Oh wow, there they are. If you go to the "Conversation" tab in the PR and look at this conversation, it still shows the old code without the docs and that's what I was looking at. SOrry!
Yep, you are right. Seems to be happening for group chats. The icon is in the right spot, but the text is not centered: I'll play with the styles tomorrow to see if I can get the group text vertically centered (if that's what we want) |
|
Yup, we want them to be vertically centered. Thanks! |
There we go! We were adding an empty second line of text, which is why no centering properties were working! |
4 participants
@nickmurray47 @tgolen, can you review?
Fixed Issues
Fixes $ https://github.com/Expensify/Expensify/issues/145380
Tests
4. Go back to the chat and delete the comment. When you leave the chat, it should go away from the LHN.Screenshots