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.

Sign up for GitHub

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

Fix #1957: Optimise code in profile_chooser_profile_view.xml #2008

Closed
wants to merge 23 commits into from
Closed

Fix #1957: Optimise code in profile_chooser_profile_view.xml #2008

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
a520170
{{Fixes #1997:Optimise code in profile chooser fragment xml}}
Oct 13, 2020
5367b77
{{Fix #1997:Optimise code in profile_chooser_profile_view.xml}}
Oct 13, 2020
b83140c
{{Fix #1957:Optimise code in profile_chooser_profile_view.xml}}
Oct 13, 2020
a0a8ee9
{{Fixes #2008: Optimise code in profile_chooser_profile_view.xml }}
Oct 14, 2020
0548713
{{Fixes #2008: Optimise code in profile_chooser_profile_view}}
Oct 16, 2020
18c6e70
{{Fixes #2008: Optimise code in profile_choose_profile
Oct 16, 2020
92b7dd9
{{Fixes #2008: Optimise code in profile_chooser_profile_view}}
Oct 16, 2020
a55a249
{{Fixes #2008: Optimise code in profile_chooser_profile_view}}
Oct 16, 2020
294cb6f
{{Fixes #2008: Optimise code in profile_chooser_profile_view}}
Oct 17, 2020
432683d
{{Fixes #2008: Optimise code in profile_chooser_profile_view}}
Oct 17, 2020
dcc636c
{{Fixes #2008: Optimise code in profile_chooser_profile_view}}
Oct 17, 2020
4714f32
{{Fixes #1957: Optimise code in profile_chooser_profile_view}}
Oct 21, 2020
f94cf17
Merge updates
Oct 21, 2020
693bb8e
{{Fix #1957: Optimise code in profile_chooser_profile_view.xml}}
Oct 23, 2020
dd1627f
Margins updated
Oct 27, 2020
9066029
Merge remote-tracking branch 'upstream/develop' into profile_chooser_…
Oct 27, 2020
1111184
Merge remote-tracking branch 'upstream/develop' into profile_chooser_…
Oct 28, 2020
ceb8501
Other changes
Oct 29, 2020
1d9d7eb
Other changes
peculiaruc Oct 29, 2020
4b4376c
Fixed Margins
peculiaruc Oct 29, 2020
aa723ef
Merge remote-tracking branch 'upstream/develop' into profile_chooser_…
Oct 29, 2020
a7878d6
Fixed Margins
peculiaruc Oct 30, 2020
7f86822
Fixed Margins
peculiaruc Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .idea/render.experimental.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

111 changes: 55 additions & 56 deletions app/src/main/res/layout-land/profile_chooser_profile_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,75 +21,74 @@
</data>

<androidx.constraintlayout.widget.ConstraintLayout
android:id="@+id/profile_chooser_item"
android:layout_width="match_parent"
android:layout_height="wrap_content">
android:layout_height="wrap_content"
android:gravity="center_horizontal"
android:orientation="vertical"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent">

<LinearLayout
android:id="@+id/profile_chooser_item"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginStart="16dp"
android:layout_marginEnd="16dp"
android:gravity="center_horizontal"
android:orientation="vertical"
app:layout_constraintEnd_toEndOf="parent"
<de.hdodenhof.circleimageview.CircleImageView
android:id="@+id/profile_avatar_img"
android:layout_width="72dp"
android:layout_height="72dp"
android:layout_marginBottom="8dp"
app:civ_border_color="@color/avatarBorder"
app:civ_border_width="1dp"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent"
app:layoutMarginBottom="@{hasProfileEverBeenAddedValue ? @dimen/space_16dp : @dimen/space_28dp}"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_16dp : @dimen/space_24dp}">
profile:src="@{viewModel.profile.avatar}" />

<de.hdodenhof.circleimageview.CircleImageView
android:id="@+id/profile_avatar_img"
android:layout_width="72dp"
android:layout_height="72dp"
android:layout_marginBottom="8dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if its a valid comment but this (marginBottom) was missed or has been adjusted somwhere else.

Copy link
Contributor Author

@peculiaruc peculiaruc Oct 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I added android:layout_marginBottom="8dp" here

app:civ_border_color="@color/avatarBorder"
app:civ_border_width="1dp"
profile:src="@{viewModel.profile.avatar}" />

<TextView
android:id="@+id/profile_name_text"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:ellipsize="end"
android:fontFamily="sans-serif-medium"
android:gravity="center"
android:maxLines="2"
android:singleLine="false"
android:text="@{viewModel.profile.name}"
android:textColor="@color/white"
android:textSize="14sp" />
<TextView
android:id="@+id/profile_name_text"
style="@style/Caption"
android:layout_marginTop="@dimen/space_8dp"
android:ellipsize="end"
android:gravity="center"
android:maxLines="2"
android:singleLine="false"
android:text="@{viewModel.profile.name}"
android:textColor="@color/white"
app:layout_constraintStart_toEndOf="@+id/profile_avatar_img"
app:layout_constraintTop_toTopOf="parent" />

<TextView
android:id="@+id/profile_last_visited"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:fontFamily="sans-serif-light"
android:gravity="center"
android:textColor="@color/white"
android:textSize="12sp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.lastLoggedInTimestampMs > 0 ? View.VISIBLE : View.GONE}"
profile:lastVisited="@{viewModel.profile.lastLoggedInTimestampMs}" />
<TextView
android:id="@+id/profile_last_visited"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:fontFamily="sans-serif-light"
android:gravity="center"
android:textColor="@color/white"
android:textSize="12sp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.lastLoggedInTimestampMs > 0 ? View.VISIBLE : View.GONE}"
app:layout_constraintStart_toEndOf="@+id/profile_avatar_img"
app:layout_constraintTop_toBottomOf="@+id/profile_name_text"
profile:lastVisited="@{viewModel.profile.lastLoggedInTimestampMs}" />

<TextView
android:id="@+id/profile_is_admin_text"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:fontFamily="sans-serif-light"
android:gravity="center"
android:text="@string/profile_chooser_admin"
android:textColor="@color/white"
android:textSize="12sp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.isAdmin ? View.VISIBLE : View.GONE}" />
</LinearLayout>
<TextView
android:id="@+id/profile_is_admin_text"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="@dimen/space_16dp"
android:fontFamily="sans-serif-light"
android:gravity="center"
android:text="@string/profile_chooser_admin"
android:textColor="@color/white"
android:textSize="12sp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.isAdmin ? View.VISIBLE : View.GONE}"
app:layout_constraintStart_toStartOf="@+id/profile_last_visited"
app:layout_constraintTop_toBottomOf="@+id/profile_last_visited" />

<View
android:id="@+id/add_profile_divider_view"
android:layout_width="0.5dp"
android:layout_height="match_parent"
android:layout_gravity="bottom"
android:layout_marginEnd="8dp"
android:background="@color/oppiaProfileChooserDivider"
android:visibility="@{hasProfileEverBeenAddedValue ? View.GONE : View.VISIBLE}"
app:layout_constraintBottom_toBottomOf="@+id/profile_chooser_item"
Expand Down
141 changes: 70 additions & 71 deletions app/src/main/res/layout-sw600dp-land/profile_chooser_profile_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,89 +20,88 @@
type="org.oppia.android.app.model.ProfileChooserUiModel" />
</data>

<LinearLayout
<androidx.constraintlayout.widget.ConstraintLayout
android:id="@+id/profile_chooser_item"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:gravity="@{hasProfileEverBeenAddedValue ? Gravity.TOP|Gravity.CENTER : Gravity.START}"
android:gravity="center_horizontal"
android:orientation="vertical"
app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_298dp}"
app:layoutMarginBottom="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_0dp}"
app:layoutMarginEnd="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_298dp}"
app:layoutMarginBottom="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_0dp}">

<LinearLayout
android:id="@+id/profile_chooser_item"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:gravity="@{hasProfileEverBeenAddedValue ? Gravity.CENTER_HORIZONTAL : Gravity.CENTER}"
android:orientation="@{hasProfileEverBeenAddedValue ? LinearLayout.VERTICAL : LinearLayout.HORIZONTAL}"
app:layoutMarginEnd="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_24dp}"
app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_24dp}"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_24dp}"
profile:ignore="RtlSymmetry">

<de.hdodenhof.circleimageview.CircleImageView
android:id="@+id/profile_avatar_img"
android:layout_width="108dp"
android:layout_height="108dp"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_48dp : @dimen/space_0dp}"
app:civ_border_color="@color/avatarBorder"
app:civ_border_width="1dp"
profile:src="@{viewModel.profile.avatar}" />
app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_298dp}"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent"
profile:ignore="RtlSymmetry">

<LinearLayout
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:gravity="@{hasProfileEverBeenAddedValue ? Gravity.CENTER_HORIZONTAL : Gravity.CENTER_VERTICAL}"
android:orientation="vertical"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_8dp : @dimen/space_0dp}"
app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_32dp}">
<de.hdodenhof.circleimageview.CircleImageView
android:id="@+id/profile_avatar_img"
android:layout_width="108dp"
android:layout_height="108dp"
app:civ_border_color="@color/avatarBorder"
app:civ_border_width="1dp"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_48dp : @dimen/space_0dp}"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent"
profile:src="@{viewModel.profile.avatar}" />

<TextView
android:id="@+id/profile_name_text"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:ellipsize="end"
android:fontFamily="sans-serif-medium"
android:gravity="center"
android:maxLines="2"
android:singleLine="false"
android:text="@{viewModel.profile.name}"
android:textColor="@color/white"
android:textSize="20sp" />
<TextView
android:id="@+id/profile_name_text"
style="@style/Heading2"
android:layout_marginTop="@dimen/space_8dp"
android:ellipsize="end"
android:gravity="center"
android:maxLines="2"
android:singleLine="false"
android:text="@{viewModel.profile.name}"
android:textColor="@color/white"
app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_32dp}"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_8dp : @dimen/space_0dp}"
app:layout_constraintStart_toEndOf="@+id/profile_avatar_img"
app:layout_constraintTop_toTopOf="parent" />

<TextView
android:id="@+id/profile_last_visited"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:fontFamily="sans-serif-light"
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we used light before so Subtitle1 doesn't seem like the correct fit.

@rt4914 looking at https://github.com/oppia/oppia-android/blob/a44791096a3e797653c3d463158db6e7662ab5cb/app/src/main/res/values/styles.xml we don't seem to have any styles with sans-serif-light. Are the current layouts wrong, or are we missing some styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Holding off on resolving this until @rt4914 can weigh in since this particular situation probably should have a new style given you found a bunch of places where the same properties are being used.

android:textColor="@color/white"
android:textSize="16sp"
android:layout_marginTop="4dp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.lastLoggedInTimestampMs > 0 ? View.VISIBLE : View.GONE}"
profile:lastVisited="@{viewModel.profile.lastLoggedInTimestampMs}" />
<TextView
android:id="@+id/profile_last_visited"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="@dimen/space_8dp"
android:fontFamily="sans-serif-light"
android:textColor="@color/white"
android:textSize="16sp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.lastLoggedInTimestampMs > 0 ? View.VISIBLE : View.GONE}"
app:layoutMarginStart="@{hasProfileEverBeenAddedValue ? @dimen/space_0dp : @dimen/space_32dp}"
app:layoutMarginTop="@{hasProfileEverBeenAddedValue ? @dimen/space_8dp : @dimen/space_0dp}"
app:layout_constraintEnd_toEndOf="@+id/profile_name_text"
app:layout_constraintHorizontal_bias="0.094"
app:layout_constraintStart_toStartOf="@+id/profile_name_text"
app:layout_constraintTop_toBottomOf="@+id/profile_name_text"
profile:lastVisited="@{viewModel.profile.lastLoggedInTimestampMs}" />

<TextView
android:id="@+id/profile_is_admin_text"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:fontFamily="sans-serif-light"
android:gravity="center"
android:text="@string/profile_chooser_admin"
android:textColor="@color/white"
android:textSize="16sp"
android:layout_marginTop="4dp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.isAdmin &amp;&amp; hasProfileEverBeenAddedValue ? View.VISIBLE : View.GONE}" />
</LinearLayout>
</LinearLayout>
<TextView
android:id="@+id/profile_is_admin_text"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginStart="@dimen/space_16dp"
android:layout_marginTop="@dimen/space_4dp"
android:fontFamily="sans-serif-light"
android:gravity="center"
android:text="@string/profile_chooser_admin"
android:textColor="@color/white"
android:textSize="16sp"
android:textStyle="italic"
android:visibility="@{viewModel.profile.isAdmin &amp;&amp; hasProfileEverBeenAddedValue ? View.VISIBLE : View.GONE}"
app:layout_constraintStart_toEndOf="@+id/profile_avatar_img"
app:layout_constraintTop_toBottomOf="@+id/profile_last_visited" />

<View
android:id="@+id/add_profile_divider_view"
android:layout_width="match_parent"
android:layout_height="0.5dp"
android:layout_marginTop="40dp"
android:layout_marginTop="@dimen/space_40dp"
android:background="@color/oppiaProfileChooserDivider"
android:visibility="@{hasProfileEverBeenAddedValue ? View.GONE : View.VISIBLE}" />
</LinearLayout>
android:visibility="@{hasProfileEverBeenAddedValue ? View.GONE : View.VISIBLE}"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent" />
</androidx.constraintlayout.widget.ConstraintLayout>
</layout>
Loading