-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix unread marking logic for mobile on visibility change #5155
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.
@meetmangukiya are you able to set up an Android emulator and test Android that way?
@Jag96 I've not managed to get Android setup working on my m1 yet 😅 |
@meetmangukiya ok, I can test Android then. Can you update the description to have screenshots for web/desktop as well to confirm no regressions? Once that's updated I'll test/review |
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.
LGTM, confirmed the tests passed on Android as well and looks good on desktop
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
isVisible
native implementation by using theAppState.currentState
to determine the state app is in.this.props.isDrawerOpen
.Fixed Issues
$ #4855
Tests
QA Steps
Tested On
Screenshots
Web
Screen.Recording.2021-09-14.at.2.57.45.PM.mov
Screen.Recording.2021-09-14.at.2.58.43.PM.mov
Mobile Web
Not needed, native mobile bug only.
Desktop
Not needed, native mobile bug only.
iOS
Simulator.Screen.Recording.-.iPhone.12.-.2021-09-09.at.12.25.59.mp4
Android
Would need help testing this.