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

[dev-client] support landscape orientation #18509

Merged
merged 5 commits into from
Aug 10, 2022

Conversation

ajsmth
Copy link
Contributor

@ajsmth ajsmth commented Aug 4, 2022

Why

Closes ENG-1804

this took a bit longer than anticipated as I ran into some orientation related bugs:

Simulator.Screen.Recording.-.iPhone.12.-.2022-08-04.at.10.31.50.mp4

How

Removed the restriction for portrait orientation on the native view controllers in the dev menu, then updated the styles of the dev menu to look decent in landscape. I had to adjust some of the calculations done by the bottom sheet to get the right snap point values, but I think it works fairly well

The big issue was related to the video posted above (on iOS only) - this was solved by "properly" setting the root view controllers' view in DevLaunchers delegate subscriber. It seems like a small change but seemed to do the trick. 🤷‍♀️

Test Plan

I tested both platforms with and without expo-screen-orientation installed - updating the app.json orientation value to be default landscape and portrait and ensured that behaviour was as expected: locked to the values when landscape / portrait, but rotates when default

Here is a quick video demonstrating:

Simulator.Screen.Recording.-.iPhone.12.-.2022-08-04.at.14.22.00.mp4

Checklist

@linear
Copy link

linear bot commented Aug 4, 2022

ENG-1804 Support landspace orientation in Dev Menu

A follow up to #14066

Due to an issue with API 26, we can't request orientation when the activity is translucent. Unfortunately, this isn't an ideal solution, cause on API 26 users may open the dev-menu in landscape orientation. There may be some visual bugs. So in the future, we may want to add support for landscape orientation.

window.rootViewController = self.reactDelegate?.createRootViewController()
window.rootViewController!.view = rootView

// NOTE: this order of assignment seems to actually have an effect on behaviour
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keith-kurak this appears to have been the root issue - pretty underwhelming!

Copy link
Contributor

Choose a reason for hiding this comment

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

WOW, I could have looked at that for years, what a find!

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Aug 4, 2022
Copy link
Contributor

@keith-kurak keith-kurak left a comment

Choose a reason for hiding this comment

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

Nice work!

I confirmed this also resolves #15835 (the bug that originally got me interested in this :-) ).

Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

seconding Keith's assessment, nice find on the view controller assignment 😮

Comment on lines -568 to -569
[[UIDevice currentDevice] setValue:@(orientation) forKey:@"orientation"];
[UIViewController attemptRotationToDeviceOrientation];
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm curious about why this is no longer needed -- with this gone, what sets the orientation when it is supposed to be restricted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could be wrong but I believe this is set at the Info.plist / OS level. I'll double check but i'm pretty sure these lines don't do anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(those plist values are set during prebuild / config plugin step)

Copy link
Contributor

@esamelson esamelson Aug 4, 2022

Choose a reason for hiding this comment

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

Ahh, I see, that makes sense. I do think there's some value from having a dev client (specifically) be able to respond dynamically to changing app.json values -- like, it would be nice if you could modify this app.json value and then see the effect right away in your dev client instead of having to make a new build. But if that is much trickier now then it probably isn't a huge deal for this particular field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if the app.json modifications work w/o rebuild - if so i don't see any harm in including it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like this only works on initial launch (i.e it will rotate to the configured orientation on launch) but the orientation is not locked unless its set in the build settings. it makes more sense to me to omit these lines

@hirbod
Copy link
Contributor

hirbod commented Aug 7, 2022

Related: #15835 and #18005

@ajsmth ajsmth requested a review from esamelson August 8, 2022 18:11
@jmarbutt
Copy link

jmarbutt commented Aug 8, 2022

Is there anything we can do to help this get released?

@@ -51,7 +51,7 @@ class DevMenuHost(application: Application) : ReactNativeHost(application) {
return packages
}

override fun getUseDeveloperSupport() = false // change it and run `yarn start` in `expo-dev-menu` to launch dev menu from local packager
override fun getUseDeveloperSupport() = true // change it and run `yarn start` in `expo-dev-menu` to launch dev menu from local packager
Copy link
Contributor

Choose a reason for hiding this comment

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

revert :)

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 9, 2022
@ajsmth ajsmth merged commit 2b5628a into main Aug 10, 2022
@ajsmth ajsmth deleted the andrew/eng-1804-support-landspace-orientation-in-dev branch August 10, 2022 15:01
Ddv0623 pushed a commit to omniaintranet/expo that referenced this pull request Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants