-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Allow App to Navigate to Last Visited Room On visiting with HOME url #6978
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
Conversation
…nnecesary updateLastRead
|
@K4tsuki Could you please add videos for other platforms as well? |
marcaaron
left a comment
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.
Looks good. There are just a couple of style inconsistencies that need to be fixed.
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.
Works great.
Fixes:
- Current issue.
- Last visited chat issue on the SearchPage.
🎀 👀 🎀 C+ reviewed
| this.scrollToListBottom(); | ||
| ReportScrollManager.scrollToBottom(); |
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.
Just noticed, Why aren't we calling scrollToBottomAndUpdateLastRead here?
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.
This is explained in the issue description:
Remove updateLastReadActionID on keyboardDidShow ( not instructed. I have tested it on android: updateLastReadActionID is being called each time user open keyboard. if user open a room and he is on a room, updateLastReadActionID is called automatically, whether he is viewing old message (scroll at the top) or not. So, when scroll at the top and user open keyboard updateLastReadActionID is unnecessary. )
I think the assumption is that we are already updating the last read when the report opens and so it is unnecessary to do it again when the keyboard opens. Feels fine to me but maybe there's some reason why we are setting it when the keyboard opens? I would guess the original intention is to just scroll to the bottom. But the excess calls to the API are because those two behaviors were stuck together before.
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 don't remember why it was added so it seems fine. But let's add proper tests so that QA can test the unread/read behavior properly.
All scenarios should be tested when messages are marked read.
- When we switch between reports.
- When we resize the window on Desktop from mobile to Web.
- etc...
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.
CC @K4tsuki See ⬆️
|
I think we are working on getting the tests passing. Is this safe to merge before then? cc @roryabraham @AndrewGable |
|
Yes, all the standard non-broken Jest tests passed. This is safe to merge with regard to unit tests. |
|
@parasharrajat I have updated the QA steps. |
|
@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
|
Not an |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @marcaaron in version: 1.1.27-2 🚀
|
|
🚀 Deployed to staging by @marcaaron in version: 1.1.27-3 🚀
|
|
🚀 Deployed to staging by @marcaaron in version: 1.1.27-3 🚀
|
|
As per steps this PR needs URL change. QA validated on Web (PASS) and It can't be QA-ed on Android app |
|
You can check this on Search Page.
|
Fixing navigate to last visited room feature when user access App using HOME url
If onyx local data is exist, App will navigate to last visited room on that device.
If onyx is not exist in the device, App will use server data, hence will open last visited room by other device.
Details
lastVisitedTimestampand it is set byupdateLastReadActionID. [Instructed]updateLastReadActionIDon create new message [Instructed]updateLastReadActionIDonkeyboardDidShow( not instructed. I have tested it on android:updateLastReadActionIDis being called each time user open keyboard. if user open a roomupdateLastReadActionIDis called automatically. When he is on top scroll (for example reading old message) then new message coming,updateLastReadActionIDis also called (IfupdateLastReadActionIDnot called here, thenupdateLastReadActionIDis necessary when he open keyboard). So thisupdateLastReadActionIDwhen keyboard open up is unnecessary.Fixed Issues
$ #6474
Tests
############
Report_UpdateLastReadis called only twice. (on developer tools)############
Report_UpdateLastReadis not called unnecesarrily.QA Steps
###############
############
Report_UpdateLastReadis called only twice. (on developer tools)############
Report_UpdateLastReadis not called unnecessarily.Tested On
Screenshots
Web
last_visit.mp4
Mobile Web
Desktop
iOS
Android