-
-
Notifications
You must be signed in to change notification settings - Fork 45
Some side nav bar changes #3419
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
Some side nav bar changes #3419
Conversation
📝 WalkthroughWalkthroughThis pull request updates the navigation drawer color scheme by introducing two new color resources and applying them across multiple layout files. The changes replace existing brand color references with a new side navigation color ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/res/layout/nav_drawer_header.xml (1)
23-23: Consider usingapp:cardBackgroundColorinstead ofandroid:backgroundTintfor CardView styling.The CardView is using
android:backgroundTintto tint its background, but the Material Design idiomatic approach is to useapp:cardBackgroundColorfor CardView background color customization. ThebackgroundTintattribute may have unexpected behavior on CardView since it tints drawable layers rather than the card background directly.<androidx.cardview.widget.CardView android:id="@+id/profile_card" android:layout_width="match_parent" android:layout_height="wrap_content" android:layout_marginTop="24dp" app:cardCornerRadius="8dp" - android:backgroundTint="@color/personal_id_profile_bg_color"> + app:cardBackgroundColor="@color/personal_id_profile_bg_color">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/layout/nav_drawer_base.xml(3 hunks)app/res/layout/nav_drawer_footer.xml(2 hunks)app/res/layout/nav_drawer_header.xml(2 hunks)app/res/layout/nav_drawer_list_item.xml(1 hunks)app/res/values/colors.xml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (9)
app/res/layout/nav_drawer_header.xml (2)
9-9: Verify text color contrast on the new darker navigation background.The background color changed from
#5D70D2to#3A42C7, a darker shade. Ensure that all text elements (usingTextAppearance.NavDrawerItemstyle at lines 47, 56) maintain sufficient contrast for WCAG accessibility standards. The white tints on icons (line 16) are appropriate, but text color definitions should be verified.The TextAppearance styles are not provided in the review context. Please confirm that the text colors defined in
@style/TextAppearance.NavDrawerItemand@style/TextAppearance.NavDrawerItem.SubTitleprovide adequate contrast (minimum 4.5:1 for normal text, 3:1 for large text per WCAG AA) against the new#3A42C7background.
1-66: Approve color scheme changes for profile header section.The header layout color updates are consistent with the PR objectives. The new darker palette creates a cohesive navigation drawer experience. White tints for the logo (line 16) ensure visibility against the dark background.
app/res/layout/nav_drawer_footer.xml (1)
9-9: Approve footer color and text updates for improved readability.The footer background color update to
cc_side_nav_coloris consistent with the broader navigation drawer color scheme. The explicit white text color assignment to the app_version label (line 101) is a good practice ensuring visibility and accessibility on the darker background.Also applies to: 101-101
app/res/layout/nav_drawer_list_item.xml (1)
12-12: Approve navigation list item background color update.The background color change to
cc_side_nav_coloris consistent with the overall navigation drawer styling. The white icon tint (line 65) ensures icon visibility on the darker background.app/res/layout/nav_drawer_base.xml (2)
5-5: Approve consistent background color updates across all navigation drawer surfaces.All background elements (outer layout, NestedScrollView, RecyclerView) are consistently updated to
cc_side_nav_color, creating a unified visual experience. The changes maintain the existing layout structure and visibility logic (e.g., signout_view remains visibility="gone" by default).Also applies to: 13-13, 71-71
51-51: Approve MaterialButton tint update for improved visual contrast.The sign-in button's
backgroundTintis updated tocc_brand_color(#5D70D2), which provides good contrast against the darker navigation background (cc_side_nav_color#3A42C7). This ensures the button remains visually distinct and actionable when the signout_view becomes visible. The tint choice maintains visual hierarchy within the navigation drawer.app/res/values/colors.xml (3)
86-86: Approve newcc_side_nav_colorresource definition.The new
cc_side_nav_color(#3A42C7) is appropriately defined in the Brand section of the CommCare color palette. The color name follows existing conventions and the darker shade complements the existingcc_brand_color(#5D70D2) for visual hierarchy.
225-225: Approve newpersonal_id_profile_bg_colorresource definition.The new
personal_id_profile_bg_color(#262a82) is appropriately defined with consistent naming matching other personal_id_ resources in the Connect colors section (lines 218–222). The darker shade creates good visual layering with other navigation colors.
1-231: Verify accessibility compliance before release.All navigation drawer text elements should be tested for WCAG contrast compliance against the new darker background palette. Consider running automated accessibility audits to ensure:
- Text in TextAppearance.NavDrawerItem styles has ≥4.5:1 contrast on
#3A42C7- Interactive elements (buttons, list items) remain clearly visible
- Touch targets remain ≥48dp
Additionally, verify the UI screenshot mentioned in the PR description confirms intended visual appearance, particularly the CardView styling change in nav_drawer_header.xml.
|
@jaypanchal-13 Please change the base branch from |
app/AndroidManifest.xml
Outdated
| xmlns:tools="http://schemas.android.com/tools" | ||
| android:versionCode="106" | ||
| android:versionName="2.61"> | ||
| android:versionName="2.62"> |
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.
Why is this change here?
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.
@Jignesh-dimagi showing commits of your name
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.
Revert the master branch rebase commit.
70d4030 to
ff2ea06
Compare
|
@damagatchi retest this please |
Product Description
Ticktet -> https://dimagi.atlassian.net/browse/CCCT-1904
Technical Summary
Screen shot
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review