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

Use debounce for onDimensionChange in withWindowDimensions HOC #2799

Merged
merged 2 commits into from
May 13, 2021
Merged

Use debounce for onDimensionChange in withWindowDimensions HOC #2799

merged 2 commits into from
May 13, 2021

Conversation

tugbadogan
Copy link
Contributor

cc @deetergp

Details

React native has a bug where it calls onDimensionChange callback with swapped dimensions for a very short time before it's called with correct dimensions. facebook/react-native#29290

When this happens, a bug in react-navigation causes left menu to be positioned in the center instead of left side. We're fixing this issue by using debounce to prevent onDimensionChange callback from being called multiple times within 100ms to let react-navigation to complete its animations and positioning elements properly.

Fixed Issues

Fixes #2180

Tests

Verified that left menu is not shown in the center when the app is loaded from background on iPad devices.

QA Steps

  1. Launch App on iPad with iOS 12
  2. Open any chat
  3. Switch orientation of iPad from landscape to portrait
  4. Switch orientation back to landscape
  5. Tap on home button on iPAD to put app into the background
  6. Open App from background
  7. Observe that left menu is not shown in the center

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

iOS

Screen.Recording.2021-05-11.at.18.35.16.420p.mov

@tugbadogan tugbadogan requested a review from a team as a code owner May 11, 2021 17:56
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team May 11, 2021 17:56
Copy link
Contributor

@jasperhuangg jasperhuangg 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 it'd be good to include a comment explaining why we're debouncing this (like you've explained in the issue) with a link to the react-native issue. Since this should be a temporary fix, it's good to have some context if we choose to remove this in the future because the bug is fixed.

Other than that, the changes look good to me, and I was able to test them as well, nice job!

@tugbadogan
Copy link
Contributor Author

I think it'd be good to include a comment explaining why we're debouncing this (like you've explained in the issue) with a link to the react-native issue. Since this should be a temporary fix, it's good to have some context if we choose to remove this in the future because the bug is fixed.

Other than that, the changes look good to me, and I was able to test them as well, nice job!

Thanks for the review @jasperhuangg, added comments and updated the PR 👍

@jasperhuangg
Copy link
Contributor

Great explanation! Thanks :)

@jasperhuangg jasperhuangg merged commit 1f71441 into Expensify:main May 13, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.43-1🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.44-0🚀

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

@tugbadogan tugbadogan deleted the tugbadogan-ios-fix-left-menu branch July 10, 2021 16:42
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.

iOS- 1.0.2-51- Left menu shown in the center and need to reload App to fix it
3 participants