-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Place the new marker when a chat becomes visible #2737
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my one question is do we need to hook into the visibility event? Doesn't this mean we will only set the new marker when the user becomes active.
I think this works, but I might expect this to work like this:
- User is not active
- New comments appear
- We should reset the new marker because they are not active
That would probably require adding similar logic to the code that handles the new actions here (which I think is the "event" we are reacting to here and maybe not the user becoming active):
But maybe I'm missing something about why we would prefer to do it this way.
Heh, @marcaaron I think that's the same thing I suggested to Alberto when he originally asked me about this. @Gonals maybe since both Marc and I had that same instinct, it might be a good reason to go with that solution over this one. Do you know how much more complex that solution would be? I do like that this PR is nice and simple, and I don't have too good of a sense over how complex the other solution would be. |
... All your questions are already answered in the body of the PR!!!
Moving the code to the other place! |
All comments addressed! |
src/libs/actions/Report.js
Outdated
|
||
// When a new message comes in, if the New marker is not already set (newMarkerSequenceNumber === 0) and, set the | ||
// marker above the incoming message. | ||
if (lodashGet(allReports, reportID).newMarkerSequenceNumber === 0 && updatedReportObject.unreadActionCount > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very possible that this report does not yet exist. And if lodashGet(allReports, reportID)
is undefined
then we will try and fail to access newMarkerSequenceNumber
.
In the case that it doesn't exist I think we would treat that as the same as having a newMarkerSequenceNumber === 0
so we should do this instead...
if (lodashGet(allReports, reportID).newMarkerSequenceNumber === 0 && updatedReportObject.unreadActionCount > 0) { | |
if (lodashGet(allReports, [reportID, 'newMarkerSequenceNumber'], 0) === 0 && updatedReportObject.unreadActionCount > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! That makes sense. Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it with Marc's suggested change.
🚀 Deployed to staging in version: 1.0.41-12🚀
|
🚀 Deployed to production in version: 1.0.44-0🚀
|
@tgolen, @marcaaron, since we were all working on this.
Details
I decided to put the code on
onVisibilityChange
inReportActionsView
because that way we keep all the "similar" logic in the same file and I think it is really easy to understand and follow what's going on.BUT, it could totally go here in
updateReportWithNewAction
too and it works just as well and it is also a logical place for it to be so... 🤷. If anyone has a strong preference, I'm fine with either.Fixed Issues
https://github.com/Expensify/Expensify/issues/163167
Tests/ QA Steps
In web:
.cash
tab. The new marker should be over the oldest of the new messages..cash
tab. The New marker should be in the same place it was before.In web and all the other platforms:
.cash
tab. The new marker should be over the oldest of the new messages..cash
tab. The New marker should be in the same place it was before.Tested On
Screenshots
Taking screenshots of the marker won't mean much.