-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added setLockOrientation for activities #1839
Added setLockOrientation for activities #1839
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.
Thank you for this! Mostly it looks great, just a few comments.
@@ -101,6 +103,9 @@ protected void onCreate(@Nullable Bundle savedInstanceState) { | |||
mHandler = ViewModelProviders.of(this).get(SocialProviderResponseHandler.class); | |||
mHandler.init(params); | |||
|
|||
if (params.lockOrientation) { |
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.
Can this be moved into AppCompatBase
so that we don't have to repeat it in each subclass?
@@ -0,0 +1,4 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
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.
I think adding a boolean resource to FirebaseUI for this is a step too far. The developer should add this in their own app and then tell us to lock the orientation accordingly.
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.
I understood the feature wrong, sorry for the inconvenience.
I hope it's alright now, thanks for the guidance.
auth/src/main/java/com/firebase/ui/auth/ui/HelperActivityBase.java
Outdated
Show resolved
Hide resolved
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.
Looks great, thank you! Just two small comments and then I will merge.
Oh and please add a line to CHANGELOG.md
describing your change.
@@ -53,4 +59,9 @@ protected void switchFragment(@NonNull Fragment fragment, | |||
protected void switchFragment(@NonNull Fragment fragment, int fragmentId, @NonNull String tag) { | |||
switchFragment(fragment, fragmentId, tag, false, false); | |||
} | |||
|
|||
@SuppressLint("all") |
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.
Do we actually need this @SuppressLint
anymore? If so can we be more specific than all
?
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.
Without @SuppressLint
when running gradlew check it gives the following error and I don't know how suppress it.
Error: You should not lock orientation of your activities, so that you can support a good user experience for any device or orientation [SourceLockedOrientationActivity]
setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
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.
So then could it be @SuppressLint("SourceLockedOrientationActivity")
?
@@ -14,8 +14,10 @@ | |||
|
|||
package com.firebase.ui.auth.ui.idp; | |||
|
|||
import android.annotation.SuppressLint; |
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.
There are no changes in this file anymore besides these two imports and some whitespace, so maybe we can revert the whole file?
auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java
Outdated
Show resolved
Hide resolved
@laurentiu-git LGTM! If tests pass I am going to merge. |
Hello, I have tried to implement the feature requested #1834 .