Delivery progress scrolling fix #3009
Conversation
…llapsing toolbar for the top part).
📝 WalkthroughWalkthroughThe pull request refactors the XML layout for the delivery progress screen. The root element is changed from a CoordinatorLayout to a NestedScrollView, and the AppBarLayout and CollapsingToolbarLayout have been removed. A new LinearLayout is introduced as the main container, which now houses an ImageView (refresh icon) and a TextView (last update), both initially set to be invisible, along with a new CardView enclosing a ConstraintLayout for payment confirmation buttons. Additionally, the previously used CardView for delivery alert has been replaced with one that includes a ConnectMediumTextView for messages. An Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/res/layout/fragment_connect_delivery_progress.xml (3)
14-18: Nested LinearLayout with White Background
A secondary LinearLayout with a vertical orientation and a hardcoded white background (#fff) is used to group early UI elements. Consider using a color resource instead of a hex value for improved consistency and maintainability.
35-47: Payment Confirmation CardView Setup
A CardView (with the IDconnect_delivery_progress_alert_tile) is created to enclose payment confirmation elements. The layout margins, corner radius, and elevation are set to create a distinct visual section.Note: Verify if the ID naming aligns with the updated design; according to the design objectives, consider if a renaming (e.g., to
cvConnectMessage) is needed for clarity.
49-102: ConstraintLayout for Payment Confirmation Elements
Within the CardView, a ConstraintLayout organizes an icon, a label, and two RoundedButton components (for “yes” and “no” actions).
- RoundedButton Text Size Concern: Both buttons use
app:roundButtonTextSize="5.0sp"while also settingandroid:textSize="18sp". This discrepancy may result in an unexpectedly small display if the custom attribute drives the actual text size in the custom view. Please verify that the intended design is achieved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/res/layout/fragment_connect_delivery_progress.xml(2 hunks)
🔇 Additional comments (7)
app/res/layout/fragment_connect_delivery_progress.xml (7)
2-7: Simplified Root Element and Viewport Setting
The root element has been changed from a CoordinatorLayout to a NestedScrollView withandroid:fillViewport="true". This simplifies the scrolling behavior by ensuring the entire content scrolls together and fills the viewport.
9-12: Added Primary LinearLayout Container
A new LinearLayout is introduced as a direct child of the NestedScrollView. This container helps in grouping all child elements for unified scrolling.
20-26: Refresh Icon ImageView Added
An ImageView for the refresh icon is added with accessibility support (usingandroid:contentDescription) and is initially hidden (android:visibility="gone"). This is appropriate for scenarios where the refresh action is conditionally displayed.
28-33: Last Update TextView Established
A TextView to display the last update information has been included and is also initially hidden. The top margin (3dp) ensures appropriate spacing.
105-117: Connect Message CardView with Negative Margin
A new RelativeLayout segment contains a CardView with the IDcvConnectMessage, which is meant to display connection-related messages. Notice the negative top margin of-35dpon this CardView—this appears to be used to create an overlapping visual effect. Please confirm that this negative margin is intentional and yields the desired UI effect.
144-152: Include Tag for Job Card Layout
An<include>tag brings in an external layout (view_job_card) with consistent margin settings. This approach promotes reusability and keeps the layout modular.
176-179: ViewPager2 Update
The ViewPager2 component is now nested directly within the primary LinearLayout with its height set towrap_content. Ensure that this setup provides adequate space for the dynamic content within the pager.
| <androidx.constraintlayout.widget.ConstraintLayout | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="5dp" | ||
| android:layout_marginTop="2dp" | ||
| android:layout_marginEnd="5dp" | ||
| android:visibility="visible" | ||
| app:layout_constraintTop_toBottomOf="@id/connect_delivery_refresh"> | ||
|
|
||
| <ImageView | ||
| android:id="@+id/connect_payment_confirm_icon" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="match_parent" | ||
| android:layout_marginBottom="15dp" | ||
| android:orientation="horizontal" | ||
| android:paddingStart="15dp" | ||
| android:paddingTop="35dp" | ||
| android:paddingEnd="15dp"> | ||
|
|
||
| <org.commcare.views.connect.connecttextview.ConnectMediumTextView | ||
| android:id="@+id/tvConnectMessage" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:src="@drawable/monetary_support" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| android:layout_marginEnd="10dp" | ||
| android:text="@string/connect_job_tile_daily_limit_description" | ||
| android:textColor="@color/connect_warning_color" | ||
| android:textSize="14sp" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Constraint Issue in ConnectMediumTextView
Within the ConstraintLayout of the cvConnectMessage CardView, the ConnectMediumTextView is defined with android:layout_width="0dp" and has a start constraint but no corresponding end constraint. For a view with a 0dp width (i.e., match constraints), it is typical to define both start and end constraints to determine its width. Consider adding an end constraint (for example, app:layout_constraintEnd_toEndOf="parent") or adjusting the width value appropriately to ensure proper layout behavior.
| android:paddingTop="35dp" | ||
| android:paddingEnd="15dp"> | ||
|
|
||
| <org.commcare.views.connect.connecttextview.ConnectMediumTextView |
There was a problem hiding this comment.
we are not using connect mediumtextview insted of that normal textview with textstyle bold
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent"/> | ||
|
|
||
| <org.commcare.views.connect.connecttextview.ConnectRegularTextView |
There was a problem hiding this comment.
we are not using connectregualartextview insted of that normal textview with textstyle normal
https://dimagi.atlassian.net/browse/CCCT-729
cross-request: dimagi/commcare-core#1455
Product Description
Replaced complex scrolling in the delivery progress page with simple whole page scrolling. The previous implementation tried to have the top cards scroll away separately than the bottom sections of the page, but users found the experience confusing.
Technical Summary
Replaced the more complex UI elements that supported the additional scrolling behavior with one big NestedScrollView.
Feature Flag
Connect
Safety Assurance
Safety story
Tested manually by scrolling the page from various points with my finger and verifying that it always worked as expected.
Automated test coverage
No automated tests for Connect yet.
QA Plan
Scrolling on the page should not do anything fancy, just scroll all the contents up and down together.
Labels and Review