Skip to content

refactor(auth): remove RelativeLayout usage #1237

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

Merged
merged 12 commits into from
Jan 13, 2021

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Dec 31, 2020

See discussion in #1217

This is a Work-In-Progress:

Anonymous Auth

BeforeAfter

Email Password

BeforeAfter

Facebook

BeforeAfter

Firebase UI

BeforeAfter

Generic IDP

BeforeAfter

Google

BeforeAfter

Multi Factor Auth

I changed this one to give it a better look because the buttons were all squeezed in there.

BeforeAfter

Passwordless

BeforeAfter

Phone Auth

BeforeAfter

@thatfiredev thatfiredev changed the title refactor(auth): remove RelativeLayout usages refactor(auth): remove RelativeLayout usage Dec 31, 2020
@thatfiredev thatfiredev marked this pull request as ready for review January 12, 2021 21:28
@thatfiredev thatfiredev requested a review from samtstern January 12, 2021 21:28
@samtstern samtstern added this to the jetpack milestone Jan 13, 2021
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

Approved with some small questions about changes to button groups.

binding.passwordlessButtons.visibility = View.GONE
binding.signedInButtons.visibility = View.VISIBLE
binding.signOutButton.visibility = View.VISIBLE
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right change? We used to hide a group of buttons now you're just hiding one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the change is correct. That group was a LinearLayout containing only a single button, so I removed it and kept the button only.

<LinearLayout
android:id="@+id/signedInButtons"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_centerInParent="true"
android:orientation="horizontal"
android:paddingLeft="16dp"
android:paddingRight="16dp"
android:visibility="gone">
<Button
android:id="@+id/signOutButton"
android:layout_marginStart="@dimen/button_horizontal_margin"
android:layout_marginEnd="@dimen/button_horizontal_margin"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_weight="1.0"
android:text="@string/sign_out"
android:theme="@style/ThemeOverlay.MyDarkButton" />
</LinearLayout>

@@ -287,13 +287,13 @@ class PhoneAuthActivity : AppCompatActivity(), View.OnClickListener {
if (user == null) {
// Signed out
binding.phoneAuthFields.visibility = View.VISIBLE
binding.signedInButtons.visibility = View.GONE
binding.signOutButton.visibility = View.GONE
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed above, but I double checked:

<LinearLayout
android:id="@+id/signedInButtons"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_centerInParent="true"
android:orientation="horizontal"
android:paddingLeft="16dp"
android:paddingRight="16dp"
android:visibility="gone"
android:weightSum="1.0">
<com.google.android.material.button.MaterialButton
android:id="@+id/signOutButton"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_weight="1.0"
android:text="@string/sign_out"
android:theme="@style/ThemeOverlay.MyDarkButton" />
</LinearLayout>

@samtstern
Copy link
Contributor

@rosariopfernandes makes sense, thank you! Also this PR taught me a lot about Guidelines and Groups in ConstraintLayout :-)

@samtstern samtstern merged commit 52007fe into firebase:master Jan 13, 2021
@thatfiredev
Copy link
Member Author

@samtstern I'm glad :)

I noticed in your RTDB PR that we could've used a guideline (in activity_sign_in.xml) to center the textfields and replace their fixed width with match_constraint (0dp). I didn't comment on it because the previous Layout was also using fixed width, so I thought we could just leave as is. But if you want to try out Guidelines, I think that'd be a good place to do so ;)

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.

2 participants