-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
UI: Change AnonymousAuthFragment Layout #1276
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.
LGTM.
Thanks for your contribution, @EdNgulele !
@EdNgulele If it's a matter of changing some properties/constraints in the existing layouts to make them look good in landscape, I think it's doable. |
Alright @rosariopfernandes, I will be working on this. Thank you! |
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 contributing! I have a question about the new choice of layouts, since we prefer to use ConstraintLayout rather than nested layouts when possible.
|
||
</LinearLayout> | ||
|
||
<androidx.constraintlayout.widget.ConstraintLayout |
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.
Is there a reason why you chose to go with a LinearLayout
as the top container but then have a ConstraintLayout
nested within?
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 most of our layouts are have been created like that 😅 :
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" | |
xmlns:tools="http://schemas.android.com/tools" | |
xmlns:app="http://schemas.android.com/apk/res-auto" | |
android:id="@+id/main_layout" | |
android:layout_width="match_parent" | |
android:layout_height="match_parent" | |
android:background="@color/grey_100" | |
android:orientation="vertical" | |
android:weightSum="4"> | |
<ProgressBar | |
android:id="@+id/progressBar" | |
android:indeterminate="true" | |
android:layout_width="match_parent" | |
android:layout_height="wrap_content" | |
android:visibility="invisible" | |
style="?android:attr/progressBarStyleHorizontal"/> | |
<LinearLayout | |
android:layout_width="match_parent" | |
android:layout_height="0dp" | |
android:layout_weight="3" | |
android:gravity="center_horizontal" | |
android:orientation="vertical"> |
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.
Yes @rosariopfernandes, I was actually trying to follow the way the other layouts were created! 👌🏽
@samtstern if you prefer a different approach, I could find a way to minimize nested layouts (when possible). But this also means doing the same exercise with the other layouts as well. Which I will also be happy to do.
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.
@EdNgulele that's ok, we can leave it as-is!
Summary
This PR updates the layout of the AnonymousAuthFragment in order to match with the design of the other layouts (e.g: EmailPasswordFragment, GenericIdpFragment, etc)
Submission Checklist
Pending tasks
Preview
Before the change:
After the change