CCCT-1775 Change Sidebar Account Setup Text To Button #3387
Conversation
Added a new button element to the XML layout.
Removed the original TextView and any references to it.
Formatted code.
Styled the button to the Figma spec, along with the relevant sidebar background colors to match. Added an arrow icon to the button. Created a new theme for the button.
Formatted the code a bit.
Capitalized the English string.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThis PR updates the navigation drawer UI styling and components. The changes include replacing the brand color scheme from Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
238-240: Consider fixing the typo in the method name.The method name
shouldShowNotiifcationshas a double 'i' and should beshouldShowNotifications. While this is a pre-existing issue, since you're already modifying this line, it would be a good opportunity to fix the typo.Apply this diff to fix the typo:
- private fun shouldShowNotiifcations(): Boolean { + private fun shouldShowNotifications(): Boolean { return PersonalIdManager.getInstance().isloggedIn() && isFeatureEnabled(NOTIFICATIONS) }Also update the call site on line 230:
binding.notificationView.visibility = - if (shouldShowNotiifcations()) View.VISIBLE else View.GONE + if (shouldShowNotifications()) View.VISIBLE else View.GONE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/res/layout/nav_drawer_base.xml(3 hunks)app/res/layout/nav_drawer_footer.xml(1 hunks)app/res/layout/nav_drawer_header.xml(1 hunks)app/res/layout/nav_drawer_list_item.xml(1 hunks)app/res/layout/nav_drawer_sublist_item.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/res/values/themes.xml(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(4 hunks)app/src/org/commcare/navdrawer/DrawerViewRefs.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
📚 Learning: 2025-05-07T06:48:32.621Z
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3070
File: app/res/values/themes.xml:80-80
Timestamp: 2025-05-07T06:48:32.621Z
Learning: The CommCare Android app doesn't require backward compatibility for support library features like the vector drawable compatibility provided by app:srcCompat. It uses android:src directly for drawable references in themes.
Applied to files:
app/res/values/themes.xml
📚 Learning: 2025-05-07T06:49:29.295Z
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3070
File: app/res/values/attrs.xml:98-113
Timestamp: 2025-05-07T06:49:29.295Z
Learning: In Android resource files, attributes like `editTextHintColor` and `editTextHintSize` can be defined once globally with format declarations and then reused in multiple styleables without repeating the format. This is the proper pattern for attribute reuse in Android's resource system.
Applied to files:
app/res/values/themes.xml
📚 Learning: 2025-05-09T06:50:50.863Z
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/layout/view_country_code_edittext.xml:2-7
Timestamp: 2025-05-09T06:50:50.863Z
Learning: The CommCare Android app uses minSdkVersion 21 (Android 5.0 Lollipop), so API 21+ attributes like paddingVertical and paddingHorizontal can be used without compatibility concerns.
Applied to files:
app/res/values/themes.xml
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Applied to files:
app/src/org/commcare/navdrawer/BaseDrawerController.kt
🧬 Code graph analysis (1)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (2)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
closeDrawer(126-128)app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)
isFeatureEnabled(22-30)
⏰ 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 (14)
app/res/values/strings.xml (1)
659-659: LGTM - Capitalization update.The capitalization change from "Sign in" to "Sign In" improves text consistency.
app/res/layout/nav_drawer_list_item.xml (1)
10-10: LGTM - Consistent color update.The background color change aligns with the broader navigation drawer redesign for improved contrast.
app/res/layout/nav_drawer_footer.xml (1)
9-9: LGTM - Consistent color update.The background color change maintains visual consistency across all navigation drawer components.
app/res/layout/nav_drawer_sublist_item.xml (1)
10-10: LGTM - Consistent color update.The background color change is consistent with other navigation drawer component updates.
app/res/layout/nav_drawer_header.xml (1)
9-9: LGTM - Consistent color update.The background color change maintains consistency with the overall navigation drawer redesign.
app/src/org/commcare/navdrawer/DrawerViewRefs.kt (2)
4-4: LGTM - Added import for Button type.The import correctly supports the type change from TextView to Button.
18-18: LGTM - Field type and resource ID updated correctly.The field type has been changed from TextView to Button, and the resource ID has been updated from
nav_drawer_sign_in_texttonav_drawer_sign_in_button, correctly aligning with the layout changes.app/res/values/themes.xml (1)
102-106: LGTM - New secondary button style added.The new
CommCare.Button.Secondarystyle provides appropriate styling for the sign-in button. ThecolorSurfaceusescc_brand_colorwhich should provide good contrast against the newconnect_blue_colordrawer background.app/res/layout/nav_drawer_base.xml (4)
3-3: LGTM - Consistent color scheme update.The background color changes from
cc_brand_colortoconnect_blue_colorare applied consistently across the root layout, NestedScrollView, and RecyclerView, providing the improved contrast mentioned in the PR objectives.Also applies to: 5-5, 13-13, 71-71
33-37: LGTM - Avatar ImageView added for logged-out state.The avatar image provides visual context for the signed-out state. The
contentDescriptionis intentionally set to@nullas this is a decorative image.
50-62: LGTM - MaterialButton implementation looks good.The MaterialButton correctly replaces the previous TextView with appropriate styling:
- Uses the new
CommCare.Button.Secondarystyle for consistent theming- Includes an end-aligned icon for visual affordance
- Proper padding and text appearance for alignment with drawer items
39-48: ThepaddingVerticalattribute is safe to use with this codebase.The codebase uses Android Gradle Plugin 8.6.1, which includes aapt2 that automatically converts
paddingVerticaltopaddingTopandpaddingBottomat build time for compatibility with devices below API 26. With minSdkVersion 23, there are no compatibility issues.Regarding
android:clickable="true"andandroid:focusable="true": the TextView has noandroid:idand is not referenced inDrawerViewRefs.kt. If no click handler is attached elsewhere, these attributes may be unnecessary, though they could serve accessibility purposes. Consider verifying their necessity.Likely an incorrect or invalid review comment.
app/src/org/commcare/navdrawer/BaseDrawerController.kt (2)
113-120: LGTM! Sign-in button implementation is correct.The replacement of
signInTextwithsignInButtonsuccessfully implements the PR objective. The click listener functionality is preserved, and the multiline formatting improves readability.
235-235: Good cleanup removing unnecessary semicolons.Kotlin doesn't require semicolons, so removing them aligns with idiomatic Kotlin style.
Also applies to: 239-239
|
@damagatchi retest this please |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Looks good to me, though when you put out a demo on it on slack it would be good to call out the color change to Brendon.
app/res/values/themes.xml
Outdated
| <item name="android:textColor">@android:color/primary_text_light</item> | ||
| </style> | ||
|
|
||
| <style name="CommCare.Button.Secondary" parent="Widget.MaterialComponents.Button"> |
There was a problem hiding this comment.
Curious why the new style here? The button looks very similar to our primary button other than not using all caps for the text.
There was a problem hiding this comment.
I'm actually really glad you brought this up!
So originally I was using a normal Button element in the nav_drawer_base XML layout, and I had issues setting the textAllCaps attribute to false because it was be overridden by other styles in the app, so the easiest way around that was to create a new style for the button.
However, when I got around to doing more styling on the button (such as adding color, a corner radius, and an icon), I eventually converted the Button element to a MaterialButton. And funny enough, I am testing my changes again right now and the default style for our MaterialButtons is exactly what we need, so the new style I created is no longer necessary.
I'm going to update this in a new commit momentarily!
Reverted changes to themes.xml and removed the new theme as it is not necessary.
Reverted changes to change the background color of the app's sidebar.
…nto task/CCCT-1775-sidebar-account-setup-button
Reverted changes to change the background color of the app's sidebar (there was one spot I missed in the last commit).
Changed the button color and renamed the color attribute to reflect the shade of blue.
|
@shubham1g5 @OrangeAndGreen Brendon reached out and asked me to revert the background color of the app sidebar back to the original color and to change the color of the button to a darker blue: 
I pushed new commits and re-requested your reviews. Brendon also said that he is looking into possible alternate color options for the button, so I may be pushing a new commit for that soon if I change the color again. |
|
Also, quick note, I reused the same dark blue that we use for barcode buttons. So I renamed the color resource to reflect the shade of blue and because it is now being reused for various different types of buttons |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Can you update the screenshot in the description to show the latest color change? Otherwise looks good to me.
conroy-ricketts
deleted the
task/CCCT-1775-sidebar-account-setup-button
branch
CCCT-1775
Product Description
I changed the "Sign In / Register" link in the sidebar to a button.
(10/31/25 edit) NOTE: This PR was recently updated to reflect design input. This is how the screen looks now:
Technical Summary
Note that creating the button on its own would of been wonky because it has no contrast to the current sidebar background color, so I went ahead and updated the background colors for the sidebar to match the design spec in the Figma document. Please let me know if there are any objections to that.
Safety Assurance
Safety story
I verified that the new button appears and tapping on it does the expected behavior.
QA Plan
It would be good for QA to verify that the button works/behaves as expected.