CCCT-625 || Improve cards UI #2925
Conversation
Improve cards UI
📝 WalkthroughWalkthroughThis pull request introduces significant UI modifications across multiple layout files and corresponding Java classes in the CommCare Connect application. The primary changes involve transitioning from The modifications span several key files:
The changes reflect a comprehensive UI enhancement strategy, focusing on improving visual presentation, interactivity, and component styling. The new implementation introduces more sophisticated UI components like The modifications maintain the existing functional logic while updating the UI components and their interactions, indicating a careful approach to UI refactoring. Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (6)
app/res/layout-land/home_screen.xml (1)
8-13: Consider using ConstraintLayout for better performance.The use of multiple layout_alignParent* attributes suggests this layout could benefit from using ConstraintLayout as the root element for better performance and easier maintenance.
app/res/layout/fragment_connect_delivery_progress.xml (2)
63-66: Consider removing redundant shadow attributesThe shadow attributes (
android:shadowColor,android:shadowDx,android:shadowDy,android:shadowRadius) are redundant sinceCardViewalready provides elevation-based shadows through theapp:cardElevationattribute.- android:shadowColor="@color/connect_light_grey_transparent" - android:shadowDx="30" - android:shadowDy="30" - android:shadowRadius="50"
107-108: Standardize text size attributesThere's an inconsistency between
android:textSize(18sp) andapp:roundButtonTextSize(5.0sp). Consider standardizing to use either one of these attributes consistently.- android:textSize="18sp" - app:roundButtonTextSize="5.0sp" + android:textSize="18sp"Also applies to: 113-114, 123-124, 131-132
app/res/layout/fragment_connect_jobs_list.xml (2)
32-48: Consider optimizing elevation and shadow valuesWhile the CardView implementation is good, the current values might be too aggressive:
- Shadow radius of 50 is quite large
- Shadow offset of 30dp in both directions is excessive
- CardView elevation of 5dp combined with large shadows might create an overly dramatic effect
Consider these more subtle values:
- android:shadowRadius="50" - android:shadowDx="30" - android:shadowDy="30" + android:shadowRadius="20" + android:shadowDx="10" + android:shadowDy="10" app:cardElevation="5dp"
78-110: Fix text size inconsistency and button stylingThere are several inconsistencies in the button implementations:
- Text size mismatch:
android:textSize="18sp"vsapp:roundButtonTextSize="5.0sp"- Both buttons have
android:textColor="@color/blue"but differentapp:roundButtonTextColorApply consistent styling:
android:layout_height="35dp" - android:textSize="18sp" - android:textColor="@color/blue" + android:textSize="16sp" + android:textColor="@color/white" - app:roundButtonTextSize="5.0sp" + app:roundButtonTextSize="16sp"Also, consider following Material Design guidelines for button pairs:
- Primary action (Yes): Filled button (current implementation is correct)
- Secondary action (No): Outlined button (current implementation is correct)
app/res/layout/home_screen.xml (1)
Line range hint
135-143: Remove commented-out button codeKeeping commented-out code can lead to confusion and maintenance issues. If this feature is planned for future implementation, it should be tracked in the issue tracker instead.
-<!-- <org.commcare.views.connect.RoundedButton--> -<!-- android:id="@+id/btnOverLimit"--> -<!-- android:layout_width="wrap_content"--> -<!-- android:layout_height="30dp"--> -<!-- android:text="@string/connect_job_tile_daily_limit"--> -<!-- app:layout_constraintBottom_toBottomOf="parent"--> -<!-- app:layout_constraintEnd_toEndOf="parent"--> -<!-- app:layout_constraintTop_toTopOf="parent"--> -<!-- app:roundButtonTextSize="4sp" />-->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/res/layout-land/home_screen.xml(2 hunks)app/res/layout/fragment_connect_delivery_progress.xml(4 hunks)app/res/layout/fragment_connect_jobs_list.xml(2 hunks)app/res/layout/home_screen.xml(1 hunks)app/src/org/commcare/connect/ConnectManager.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(2 hunks)
🔇 Additional comments (10)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (3)
22-22: LGTM: Import addition aligns with UI improvements
The addition of CardView import is consistent with the UI enhancement objectives.
65-65: Verify smooth visual transitions with CardView
The migration from ConstraintLayout to CardView is a good improvement for visual hierarchy. However, please ensure:
- Any existing animations or transitions still work smoothly
- The elevation and shadows are consistent with other cards in the app
✅ Verification successful
CardView migration appears safe - no animation dependencies found
The change from ConstraintLayout to CardView appears safe as there are no animations or transitions tied to this view. The search across the codebase shows that CardView is consistently used in other connect-related fragments, making this change align with the existing pattern. Since no animation or transition code was found in the fragment or related layout files, the migration should not cause any visual disruption.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for animation-related code for the connect_alert_tile
rg -l "connect_alert_tile.*\.(animate|transition)"
# Search for other CardView usage patterns in the codebase
rg "CardView.*connect"
Length of output: 624
Script:
#!/bin/bash
# Search for any animation or transition code in the ConnectJobsListsFragment
ast-grep --pattern 'animate($$$)' app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
rg "transition|animation" app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
# Check layout XML for any animation or transition attributes
fd "connect.*\.xml" app/res/layout --exec rg "android:animation|android:transition|@anim|@transition" {}
Length of output: 310
Line range hint 164-171: Verify ConnectManager compatibility with CardView
The updateSecondaryPhoneConfirmationTile method interacts with ConnectManager. Please ensure:
- ConnectManager.updateSecondaryPhoneConfirmationTile properly handles CardView attributes
- Click handlers and visibility changes work as expected with the new CardView
✅ Verification successful
CardView handling is properly implemented and compatible
The verification shows that ConnectManager correctly handles the CardView:
ConnectManager.updateSecondaryPhoneConfirmationTile()properly manages visibility withtile.setVisibility()- Click handlers are correctly set up and passed through from the fragment
- The view hierarchy is maintained with proper view finding using
tile.findViewById()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ConnectManager implementation for CardView compatibility
ast-grep --pattern 'class ConnectManager {
$$$
updateSecondaryPhoneConfirmationTile($_, $_, $_, $_) {
$$$
}
$$$
}'
# Search for any similar CardView visibility patterns
rg "CardView.*setVisibility|visibility.*CardView"
Length of output: 180
Script:
#!/bin/bash
# Let's try a broader search for ConnectManager class and its methods
rg -A 5 "class ConnectManager"
# Search for updateSecondaryPhoneConfirmationTile method
rg -A 10 "updateSecondaryPhoneConfirmationTile"
# Look for CardView related code in the fragment
fd -e java ConnectJobsListsFragment.java --exec cat {}
Length of output: 23595
app/res/layout-land/home_screen.xml (1)
36-37: Verify visibility state handling.
The CardView has android:visibility="gone" while its child ConstraintLayout has android:visibility="visible". This might cause unexpected behavior.
Also applies to: 47-47
app/res/layout/fragment_connect_delivery_progress.xml (1)
Line range hint 202-210: LGTM! TabLayout implementation follows Material Design guidelines
The TabLayout implementation is well-structured with appropriate styling and custom indicator.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (1)
50-50: LGTM! Type changes align with UI improvements
The variable type changes from TextView to ConnectRegularTextView and RoundedButton are consistent with the UI improvements in the layout file.
Also applies to: 97-97, 103-103
app/src/org/commcare/connect/ConnectManager.java (1)
Line range hint 297-315: LGTM! Method changes improve flexibility and UI consistency
The changes to updateSecondaryPhoneConfirmationTile are well-implemented:
- More flexible parameter type (View instead of ConstraintLayout)
- Consistent use of RoundedButton matching the UI improvements
app/res/layout/fragment_connect_jobs_list.xml (2)
8-8: LGTM: Proper background color implementation
Using a color resource for the background is the correct approach.
127-134: LGTM: Well-configured RecyclerView
The RecyclerView implementation follows best practices with proper constraints and preview support.
app/res/layout/home_screen.xml (1)
12-30: Maintain consistent card styling across layouts
The shadow and elevation values should be optimized similar to the suggestion in fragment_connect_jobs_list.xml.
| <ImageView | ||
| android:id="@+id/connect_secondary_phone_icon" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:src="@drawable/phone_heart" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent" | ||
| app:tint="@color/black" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content description for phone icon
The ImageView is missing a content description, which is important for accessibility.
<ImageView
android:id="@+id/connect_secondary_phone_icon"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
+ android:contentDescription="@string/connect_secondary_phone_icon_description"
android:src="@drawable/phone_heart"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent"
app:tint="@color/black" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ImageView | |
| android:id="@+id/connect_secondary_phone_icon" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:src="@drawable/phone_heart" | |
| app:layout_constraintBottom_toBottomOf="parent" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintTop_toTopOf="parent" | |
| app:tint="@color/black" /> | |
| <ImageView | |
| android:id="@+id/connect_secondary_phone_icon" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:contentDescription="@string/connect_secondary_phone_icon_description" | |
| android:src="@drawable/phone_heart" | |
| app:layout_constraintBottom_toBottomOf="parent" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintTop_toTopOf="parent" | |
| app:tint="@color/black" /> |
Improve cards UI
Summary
https://dimagi.atlassian.net/browse/CCCT-625
Feature Flag
Product Description
PR Checklist
Automated test coverage
Safety story