Skip to content

Conversation

@jaypanchal-13
Copy link
Contributor

Product Description

Ticktet -> https://dimagi.atlassian.net/browse/CCCT-1904

Technical Summary

Screen shot

image

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Labels and Review

  • Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
  • Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@coderabbitai
Copy link

coderabbitai bot commented Nov 13, 2025

📝 Walkthrough

Walkthrough

This 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 (cc_side_nav_color #3A42C7) in four navigation drawer layout files, update a CardView tint to use a new profile background color (personal_id_profile_bg_color #262a82), and add explicit white text color to the app version label. No structural or behavioral changes are made.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

  • Verify the new color hex values are intentional and visually appropriate (#3A42C7 and #262a82)
  • Confirm all navigation drawer layout files have been consistently updated with the new color references
  • Check that the white text color added to the app_version TextView provides sufficient contrast against the new background

Suggested labels

QA Note

Suggested reviewers

  • OrangeAndGreen
  • Jignesh-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is largely incomplete with most critical sections left empty or containing only placeholder comments. Fill in the Safety story, Automated test coverage, and QA Plan sections with specific details about testing and validation of the color changes.
Title check ❓ Inconclusive The title 'Some side nav bar changes' is vague and lacks specificity about what changes were made to the side navigation bar. Provide a more specific title that describes the actual change, such as 'Update side navigation bar colors' or 'Change sidebar menu color scheme to cc_side_nav_color'.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch CCCT-1904-change-sidebar-menu-color

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 using app:cardBackgroundColor instead of android:backgroundTint for CardView styling.

The CardView is using android:backgroundTint to tint its background, but the Material Design idiomatic approach is to use app:cardBackgroundColor for CardView background color customization. The backgroundTint attribute 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

📥 Commits

Reviewing files that changed from the base of the PR and between a8694a0 and ec4968e.

📒 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 #5D70D2 to #3A42C7, a darker shade. Ensure that all text elements (using TextAppearance.NavDrawerItem style 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.NavDrawerItem and @style/TextAppearance.NavDrawerItem.SubTitle provide adequate contrast (minimum 4.5:1 for normal text, 3:1 for large text per WCAG AA) against the new #3A42C7 background.


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_color is 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_color is 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 backgroundTint is updated to cc_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 new cc_side_nav_color resource 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 existing cc_brand_color (#5D70D2) for visual hierarchy.


225-225: Approve new personal_id_profile_bg_color resource 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.

OrangeAndGreen
OrangeAndGreen previously approved these changes Nov 17, 2025
@Jignesh-dimagi
Copy link
Contributor

Jignesh-dimagi commented Nov 18, 2025

@jaypanchal-13 Please change the base branch from master to commcare_2.61

@jaypanchal-13 jaypanchal-13 changed the base branch from master to commcare_2.61 November 18, 2025 05:12
xmlns:tools="http://schemas.android.com/tools"
android:versionCode="106"
android:versionName="2.61">
android:versionName="2.62">
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

@jaypanchal-13 jaypanchal-13 force-pushed the CCCT-1904-change-sidebar-menu-color branch from 70d4030 to ff2ea06 Compare November 18, 2025 11:53
@dimagi dimagi deleted a comment from Jignesh-dimagi Nov 18, 2025
@jaypanchal-13
Copy link
Contributor Author

@damagatchi retest this please

@jaypanchal-13 jaypanchal-13 merged commit 54d9845 into commcare_2.61 Nov 19, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants