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

[expo-screen-orientation] Fix getting initial orientation. #8727

Merged

Conversation

lukmccall
Copy link
Contributor

@lukmccall lukmccall commented Jun 8, 2020

Why

Fixes #8210.

How

  • Initializes current screen orientation in the constructor of the EXScreenOrientationRegisty.
  • Saves the last known orientation mask when the app is moved to the background and restores it when the app is moved to the foreground.

Test Plan

  • bare-expo ✅
  • client ✅

Changelog

  • Fix ScreenOrientation. getOrientationAsync returning a wrong value when the application is starting.

@lukmccall lukmccall requested a review from sjchmiela June 8, 2020 14:18
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2020

Native Component List for this branch is ready

Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong — this sounds like a good fix to the "initial orientation" problem, but I'm a little bit afraid that setting the mask to more rigid only after the app is foregrounded… could it mean that when a user taps on an icon they can see the rotation being executed (if the device would have been rotated while the app was in background).

Leaving this behavior of disabling lock when the app backgrounds may be useful for brownfield apps (eg. RN app lock shouldn't automatically impact app after being hidden — or maybe it should?), but if this feature would cause the app to rotate in background or something maybe we should rethink this. Does it rotate in background?

@lukmccall
Copy link
Contributor Author

could it mean that when a user taps on an icon they can see the rotation being executed (if the device would have been rotated while the app was in background).

Without this fix, the user could see the rotation in the background. Now, it'll freeze in the last set orientation. So, when you click on the home button, your app will be displayed in the correct orientation.

Leaving this behavior of disabling lock when the app backgrounds may be useful for brownfield apps (eg. RN app lock shouldn't automatically impact app after being hidden — or maybe it should?), but if this feature would cause the app to rotate in background or something maybe we should rethink this. Does it rotate in background?

No, it doesn't rotate in the background. It will work like other native apps.

@lukmccall lukmccall requested a review from sjchmiela June 9, 2020 14:02
@sjchmiela
Copy link
Contributor

Don't forget to add a note about this change in behavior (breaking, imho) that now the module will keep the lock active when the app backgrounds!

Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • changelog! 👏 👏 👏

@lukmccall lukmccall force-pushed the @lukmccall/expo-screen-orientation/fix-initial-orientation branch from 93ce253 to 205f00b Compare June 9, 2020 16:14
@lukmccall lukmccall force-pushed the @lukmccall/expo-screen-orientation/fix-initial-orientation branch from 205f00b to 599ffdb Compare June 15, 2020 14:10
@lukmccall lukmccall merged commit 5dffc10 into master Jun 15, 2020
@lukmccall lukmccall deleted the @lukmccall/expo-screen-orientation/fix-initial-orientation branch June 15, 2020 15:18
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.

locking screen orientation buggy in SDK37
2 participants