Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Messages marked as read incorrectly for last chat #2553

Merged
merged 4 commits into from
Apr 27, 2021
Merged

Fix: Messages marked as read incorrectly for last chat #2553

merged 4 commits into from
Apr 27, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Apr 23, 2021 •
edited
Loading

Please review @Julesssss

Details

Kindly, see the detailed proposal here #2539 (comment)

Fixed Issues

Fixes #2539

Tests / QA Steps

1. Send someone a message
2. Navigate back to the LHN
3. Don't select a different chat, stay on the LHN
4. Have the person send you a message back
5. Their chat row in the LHN gets bolded to indicate it's unread
6. The row stays bolded until you tap into it

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web / Mobile Web / Desktop

video.mp4

iOS / Android

1619212041981.mp4

Julesssss
Julesssss previously approved these changes Apr 23, 2021
View reviewed changes
@parasharrajat parasharrajat marked this pull request as ready for review April 23, 2021 21:13
@parasharrajat parasharrajat requested a review from a team as a code owner April 23, 2021 21:13
@MelvinBot MelvinBot requested review from johnmlee101 and removed request for a team April 23, 2021 21:13
@parasharrajat
Copy link
Member Author

Ready for review.

johnmlee101
johnmlee101 reviewed Apr 26, 2021
View reviewed changes
Copy link
Contributor

@johnmlee101 johnmlee101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, but I'll defer to @Julesssss for final review.

@johnmlee101
Copy link
Contributor

Merge conflicts just popped up

@parasharrajat
Copy link
Member Author

@johnmlee101 Just resolved it. Thanks.

Julesssss
Julesssss previously approved these changes Apr 27, 2021
View reviewed changes
@Julesssss
Copy link
Contributor

@parasharrajat a final conflict to resolve

@parasharrajat parasharrajat force-pushed the parasharrajat/unreadchat branch from 5d619e3 to 388543d Compare April 27, 2021 14:48
@parasharrajat
Copy link
Member Author

@Julesssss Just cleared up the Conflicts. It seems there were some revert commits so I force pushed it.

Julesssss
Julesssss approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified again, LGTM

@Julesssss Julesssss merged commit dc62b89 into Expensify:main Apr 27, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@Julesssss Julesssss added the Weekly KSv2 label Apr 27, 2021
@Julesssss Julesssss changed the title Fix: Messages marked as read incorrectly for last chat Fix: Messages marked as read incorrectly for last chat [Pay on 4th May] Apr 27, 2021
@Julesssss Julesssss changed the title Fix: Messages marked as read incorrectly for last chat [Pay on 4th May] Fix: Messages marked as read incorrectly for last chat Apr 27, 2021
@Julesssss Julesssss removed the Weekly KSv2 label Apr 27, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.32-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

marcaaron
marcaaron reviewed May 4, 2021
View reviewed changes
src/pages/home/report/ReportActionsView.js
@@ -125,6 +140,9 @@ class ReportActionsView extends React.Component {
// The last sequenceNumber of the same report has changed.
const previousLastSequenceNumber = lodashGet(lastItem(prevProps.reportActions), 'sequenceNumber');
const currentLastSequenceNumber = lodashGet(lastItem(this.props.reportActions), 'sequenceNumber');
const shouldRecordMaxAction = Visibility.isVisible()
&& !(this.props.isDrawerOpen && this.props.isSmallScreenWidth);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is a bit hard to follow... can we please get rid of the !(this && that) logic so that this more accurately presents which scenarios we want to mark the report as read?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following your comment here. This check make sure that we don't mark the actions as read when drawer is open on small screens.

On the other hand, Drawer is always open on web so we want to let it read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a personal preference, but it took me some time to parse

e.g. we were to explain this to someone we might say we want to mark as read in two situations

  1. The app is visible and we are not on a small screen

OR

  1. The app is visible, we are on a small screen, and the drawer is closed

Compare this to saying we want to mark as read when:

  1. The app is visible AND all other situations except when the drawer is open and the screen is small.

Maybe just me, but this is easier to understand if you see it like this...

// Always record on large screens. Only record on small screens when the sidebar is closed.
const shouldRecordMaxAction = Visibility.isVisible() 
    && (!this.props.isSmallScreenWidth || !this.props.isDrawerOpen)

lmk if it looks the same to you 😂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Will update.

@parasharrajat parasharrajat mentioned this pull request May 7, 2021
Merged
5 tasks
@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@parasharrajat parasharrajat deleted the parasharrajat/unreadchat branch November 20, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcaaron marcaaron marcaaron left review comments

@johnmlee101 johnmlee101 johnmlee101 left review comments

@Julesssss Julesssss Julesssss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Messages marked as read incorrectly for last chat [Pay on 4th May]
5 participants
@parasharrajat @johnmlee101 @Julesssss @OSBotify @marcaaron