-
Notifications
You must be signed in to change notification settings - Fork 499
Replace spacing near scroll views with overscroll (COMMUNTIY) #2884
Replace spacing near scroll views with overscroll (COMMUNTIY) #2884
Conversation
Note that this changes the total padding of the content in the debuglog fragment from 20dp to 24dp for consistency within the layout.
@fynngodau Thanks. Internal Tracking ID: EXPOSUREAPP-6605 Corona-Warn-App Open Source Team |
I added fixes for the following layouts, which I initially overlooked:
|
This partially reverts commit 85e4650 to avoid conflicts with corona-warn-app#2945.
I undid the change to the Detailed Information about Data Sharing screen to avoid conflict with #2945. |
Also changes margin of button to match that of contact journal onboarding
One more screen:
Also fixes the margin of the Check In onboarding Agree button to match that of the Open Journal button in the Contact Journal onboarding. |
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.
Overall looks pretty good, but I would like to take this chance to align all bottom paddings :)
What do you think?
@@ -24,7 +24,10 @@ | |||
android:layout_width="0dp" | |||
android:layout_height="0dp" | |||
android:fillViewport="true" | |||
app:layout_constraintBottom_toBottomOf="@+id/guideline_bottom" | |||
android:paddingBottom="@dimen/spacing_small" |
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 would align the bottom padding on this kind of fragments, I would go with @dimen/spacing_normal
as it is here: https://github.com/corona-warn-app/cwa-app-android/pull/2884/files#diff-99108bde996a7dd32ce38fb5594a027efd59819f2b88d116dcc7b8f3eaced7ebR30
@@ -24,7 +24,10 @@ | |||
android:layout_width="0dp" | |||
android:layout_height="0dp" | |||
android:fillViewport="true" | |||
app:layout_constraintBottom_toBottomOf="@+id/guideline_bottom" | |||
android:paddingBottom="@dimen/spacing_small" |
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.
Same as above.
android:focusable="true" | ||
app:layout_constraintBottom_toBottomOf="@+id/guideline_bottom" | ||
android:paddingBottom="@dimen/spacing_small" |
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.
Same as above.
@@ -30,7 +30,9 @@ | |||
android:layout_width="match_parent" | |||
android:layout_height="match_parent" | |||
android:fillViewport="true" | |||
android:paddingBottom="@dimen/guideline_bottom"> | |||
android:paddingBottom="@dimen/guideline_bottom" |
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.
Same as above.
@@ -30,7 +30,9 @@ | |||
android:layout_width="match_parent" | |||
android:layout_height="match_parent" | |||
android:fillViewport="true" | |||
android:paddingBottom="@dimen/guideline_bottom"> | |||
android:paddingBottom="@dimen/guideline_bottom" |
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.
Same as above.
@@ -31,7 +31,10 @@ | |||
android:layout_width="0dp" | |||
android:layout_height="0dp" | |||
android:fillViewport="true" | |||
app:layout_constraintBottom_toBottomOf="@+id/guideline_bottom" | |||
android:paddingBottom="@dimen/spacing_small" |
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.
Same as above.
@@ -32,7 +32,10 @@ | |||
android:layout_width="0dp" | |||
android:layout_height="0dp" | |||
android:fillViewport="true" | |||
app:layout_constraintBottom_toBottomOf="@+id/guideline_bottom" | |||
android:paddingBottom="@dimen/spacing_small" |
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.
Same as above.
@@ -27,7 +27,10 @@ | |||
android:id="@+id/scrollview" | |||
android:layout_width="0dp" | |||
android:layout_height="0dp" | |||
app:layout_constraintBottom_toTopOf="@+id/guideline_bottom" | |||
android:paddingBottom="@dimen/spacing_small" |
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.
Same as above.
@@ -93,7 +93,10 @@ | |||
<ScrollView | |||
android:layout_width="0dp" | |||
android:layout_height="0dp" | |||
app:layout_constraintBottom_toBottomOf="@id/guideline_bottom" | |||
android:paddingBottom="@dimen/spacing_small" |
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.
Same as above. But there seems to be already more space.
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.
The answer is here:
cwa-app-android/Corona-Warn-App/src/main/res/layout/fragment_statistics_explanation.xml
Line 431 in e1ec946
android:layout_marginBottom="23dp" |
That's an odd margin… we should remove that before increasing the scroll view's padding.
@@ -31,7 +32,9 @@ | |||
android:id="@+id/check_in_onboarding_scroll_view" | |||
android:layout_width="match_parent" | |||
android:layout_height="0dp" | |||
android:layout_marginBottom="20dp" | |||
android:paddingBottom="@dimen/spacing_small" |
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.
Seems fine, as the floating button is below.
@harambasicluka Agreed – I have pushed a change updating all the places you pointed out. |
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.
Very nice! Thanks for fixing.
As previously discussed in #1882 (comment), this PR removes white space near scroll views, mostly at the bottom, and instead adds that space inside the scroll view (so it scrolls with the content). This makes the app feel a lot better.
Here you can see some examples:
These views have been fixed that way:
The error report screen has also been fixed to remove the white space between the elevated, sticky content and the scrollable content. Note that here, I changed the total bottom padding of the scrollable content from
20dp
to24dp
for consistency within the layout.Additionally, I fixed the button in the check in layout to have the same margin to top and bottom as the Open Journal button in the Contact Journal onboarding / information screen.
Internal Tracking ID: EXPOSUREAPP-6605