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.

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

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

meetmangukiya
Copy link
Contributor

@meetmangukiya meetmangukiya commented Sep 9, 2021

Details

  1. Fix the isVisible native implementation by using the AppState.currentState to determine the state app is in.
  2. Only mark as read when the report is actually open by checking this.props.isDrawerOpen.

Fixed Issues

$ #4855

Tests

  1. Open any report.
  2. Mark any message as unread.
  3. Background or quit the app
  4. Reopen the app and report
  5. Verify the message remains unread
  6. Repeat the steps for web, desktop and mobile web to see this does not break any current mark-read functionality on these platforms.

QA Steps

  1. Open any report.
  2. Mark any message as unread.
  3. Background or quit the app
  4. Reopen the app and report
  5. Verify the message remains unread
  6. Repeat the steps for web, desktop and mobile web to see this does not break any current mark-read functionality on these platforms.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

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.

@meetmangukiya meetmangukiya requested a review from a team as a code owner September 9, 2021 07:11
@MelvinBot MelvinBot requested review from Jag96 and removed request for a team September 9, 2021 07:11
Copy link
Contributor

@Jag96 Jag96 left a 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?

@meetmangukiya
Copy link
Contributor Author

@Jag96 I've not managed to get Android setup working on my m1 yet 😅

@Jag96
Copy link
Contributor

Jag96 commented Sep 10, 2021

@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

Copy link
Contributor

@Jag96 Jag96 left a 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

@Jag96 Jag96 merged commit f0cc89b into Expensify:main Sep 15, 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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Jag96 in version: 1.0.99-5 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.0-2 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants