Pm rounded button #2897
Pm rounded button #2897
Conversation
There was a problem hiding this comment.
@pm-dimagi a couples of questions:
- I noticed that you applied the
CustomButtonStyleto buttons that had specific styles (different from the default style), did you test the affected features to validate the visual impact? - Here is the link too the figma https://www.figma.com/design/UWjp1wRNAO0o9q2llwDKMc/CommCare-Cosmetic-Changes-Post-Nov-'22Rebrand?node-id=111-2&node-type=canvas&t=i0Tr9NVN0YY4o3SC-0
- This is a silly one, but I'm not sure about the rounded corners
DONEbutton. I think because it spans the entire width of the parent view, it doesn't give a good visual effect. Did that design come form the marketing team?
|
@pm-dimagi This should not include changes from #2892 to minimise the reviewer effort, as such you should change the base branch here to the other PR instead of |
|
@pm-dimagi An essential part of creating PR is to ensure all PR Checklist boxes being thought of and ticked. (You should tick all items of the checklist to communicate that you have thought and taken proper action on each of the items) Also wants to flag that your PR ticks the Also it would be great if you can respond to Ahmad's comment above here as I am curious about the points raised by Ahmad as well. |
pm-dimagi
added
the
High Risk
…nto pm_rounded_button # Conflicts: # app/res/values/colors.xml
There was a problem hiding this comment.
@pm-dimagi I started to review it but stopped in between as I think there are various places where the button's existing style has been removed, this would be impractical to review on code level without either having screenshots of the impacted place Or a reasoning on why you think removing old style would not cause non-intented changes with that specific button OR a comment in Safety story mentioning that you have made sure that the button doesn't appear in some weird fashion for it's respective usage on UI. If you have trouble figuiring out where a specific xml layout is used feel free to ping the mobile slack.
Regarding the safety story, I think you will need to test the app out at several places and all different widgets. I good way to do that is to run the instrumentations tests locally on your phone, specifically the FormEntryTest
Also it would be great if you can respond to Ahmad's comment above here as I am curious about the points raised by Ahmad as well.
| <Button | ||
| style="@style/MapButtonGreen" | ||
| <com.google.android.material.button.MaterialButton | ||
| android:backgroundTint="@color/green_500" |
There was a problem hiding this comment.
The old styled defined a selector that changes based on pressed button state, any reason we are no longer doing that ?
There was a problem hiding this comment.
I don't see anything in response to my comment above, as so curious why was this resolved ?
There was a problem hiding this comment.
I don't see anything in response to my comment above, as so curious why was this resolved ?
There was a problem hiding this comment.
This is updated in the last commit i have created the new style for this which is of same behaviour as the previous one
There was a problem hiding this comment.
can you please link me to the style you are referring to ?
There was a problem hiding this comment.
shubham by mistake i have made that correction in title bar color change pr . You can check the code over here
|
@shubham1g5 @avazirna this is to confirm that I have removed the old styles of the buttons as I have to give the new style of rounded shape but I have made sure that the other properties of that style is incoperatesd in the xml itself like for the MapButtonGreen I have given the green color speciic to the Material button instead of passing through the style. |
|
@damagatchi retest this please |
Looking into these now |
avazirna
force-pushed
the
pm_rounded_button
branch
from
c06692c to
67d2919
Compare
| @@ -325,10 +325,10 @@ | |||
| <item name="android:textStyle">bold</item> | |||
| </style> | |||
|
|
|||
| <style name="CustomButtonStyleOutline" parent="Widget.MaterialComponents.Button.OutlinedButton"> | |||
| <item name="shapeAppearanceOverlay">@style/RoundedButtonStyle</item> | |||
| <style name="NegativeButtonStyle" parent="Widget.MaterialComponents.Button.OutlinedButton"> | |||
There was a problem hiding this comment.
@shubham1g5 I think this is more appropriate
|
@damagatchi retest this please |
|
@coderabbitai full review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (45)
app/res/drawable/commcare_actionbar_logo_spacing.xml (1)
1-7: LGTM! Consider adding a commentThe implementation follows Material Design spacing guidelines with the 16dp margin and uses the appropriate layer-list drawable for this use case.
Consider adding a comment to explain the purpose of this spacing:
<?xml version="1.0" encoding="utf-8"?> +<!-- Adds 16dp right margin to the CommCare action bar logo for proper spacing in the action bar --> <layer-list xmlns:android="http://schemas.android.com/apk/res/android"> <item android:drawable="@drawable/commcare_actionbar_logo" android:right="16dp"/> </layer-list>app/res/layout/screen_form_dump.xml (1)
Line range hint
48-62: Consider adding style and accessibility attributes.The transition to MaterialButton components looks good and aligns with the PR objectives for rounded buttons. Consider these improvements:
- Add a common style attribute for consistent theming:
<com.google.android.material.button.MaterialButton android:id="@+id/screen_bulk_form_dump" + style="@style/Widget.MaterialComponents.Button" android:layout_width="match_parent"
- Add content descriptions for accessibility:
<com.google.android.material.button.MaterialButton android:id="@+id/screen_bulk_form_dump" + android:contentDescription="@string/dump_forms_accessibility_description" android:layout_width="match_parent"app/res/drawable/check_update.xml (1)
8-8: Good move to resource-based color, but consider renaming the color resource.The change from hardcoded color to a resource reference improves maintainability. However, the resource name
square_button_icon_colorseems inconsistent with the PR's goal of converting to rounded buttons.Consider renaming the color resource to something more neutral like
button_icon_colororprimary_button_icon_colorto better reflect its usage with rounded buttons.app/res/drawable/install_stop.xml (1)
2-5: Consider optimizing vector dimensionsThe vector dimensions have been significantly increased (232dp → 300dp) with an unusual aspect ratio (300:233). Large vector drawables can impact rendering performance. Consider:
- Maintaining the original size if possible
- Using standard dimensions (e.g., 24dp for material icons)
- Preserving a standard aspect ratio (1:1 for most icons)
- android:width="300dp" - android:height="233dp" - android:viewportWidth="300" - android:viewportHeight="233"> + android:width="24dp" + android:height="24dp" + android:viewportWidth="24" + android:viewportHeight="24">app/res/layout/blue_outlined_button.xml (1)
12-12: Consider standardizing padding valueThe hardcoded padding value of 10dp might be better defined as a dimension resource to maintain consistency across the app.
Consider moving the padding value to
dimens.xml:- android:padding="10dp" + android:padding="@dimen/button_padding"app/res/layout/missing_multimedia_layout.xml (2)
19-22: Consider converting pixel units to density-independent pixels (dp).While the image resource change looks good, the layout uses fixed pixel units (px) which may not scale properly across different screen densities. Consider converting these to density-independent pixels (dp):
- android:paddingLeft="20px" - android:layout_marginRight="20px" - android:paddingTop="20px" - android:paddingBottom="0px" - android:maxHeight="135px" - android:maxWidth="663px" + android:paddingLeft="10dp" + android:layout_marginRight="10dp" + android:paddingTop="10dp" + android:paddingBottom="0dp" + android:maxHeight="68dp" + android:maxWidth="332dp"
Issue found with inconsistent ID reference pattern
The ID
screen_multimedia_retryis referenced in both layouts but is never properly defined with@+id/. Both files use@id/screen_multimedia_retrywhich assumes the ID is defined elsewhere, but theskip_verification_buttonreferences it with@+id/in itslayout_aboveattribute. This creates an inconsistent and potentially problematic ID reference pattern.
- In
blank_missing_multimedia_layout.xml: Button usesandroid:id="@id/screen_multimedia_retry"but is referenced as@+id/screen_multimedia_retry- In
missing_multimedia_layout.xml: Same inconsistency exists🔗 Analysis chain
Line range hint
33-40: Verify the button ID reference.The button conversion looks good, but let's verify that
@id/screen_multimedia_retryis defined elsewhere in the project since it's not using the@+id/notation to create a new ID.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the definition of screen_multimedia_retry ID # Expected: Find where this ID is originally defined with @+id/ # Search for the ID definition in resource files rg -t xml "screen_multimedia_retry" app/res/Length of output: 455
Script:
#!/bin/bash # Let's check the content of both files to understand the full context cat app/res/layout/blank_missing_multimedia_layout.xml app/res/layout/missing_multimedia_layout.xmlLength of output: 4566
app/res/layout/prompt_view.xml (1)
40-60: Consider creating reusable button stylesTo maintain consistency across the app and align with the PR's objective of standardizing rounded buttons, consider:
- Creating dedicated styles in
styles.xml:
- Primary action button style
- Secondary action button style
- Extracting hardcoded dimensions to
dimens.xml- Using semantic color resources instead of direct colors
Example style definition:
<!-- styles.xml --> <style name="Widget.YourApp.Button.Primary" parent="Widget.Material3.Button"> <item name="android:minHeight">@dimen/button_min_height</item> <item name="android:minWidth">@dimen/button_min_width</item> <item name="android:textSize">@dimen/font_size_xlarge</item> <item name="android:layout_margin">@dimen/standard_spacer</item> <item name="backgroundTint">@color/cc_attention_positive_color</item> </style>app/res/layout/hierarchy_layout.xml (2)
Line range hint
39-83: Consolidate button styles to improve maintainability.The three buttons share identical styling attributes. Consider extracting these common attributes into a dedicated style to improve maintainability and ensure consistency.
Create a new style in
styles.xml:<style name="HierarchyButton" parent="Widget.MaterialComponents.Button.OutlinedButton"> <item name="android:layout_width">wrap_content</item> <item name="android:layout_height">wrap_content</item> <item name="android:textSize">@dimen/text_medium</item> <item name="android:layout_weight">1</item> </style>Then simplify the button declarations:
- <com.google.android.material.button.MaterialButton - android:id="@+id/jumpPreviousButton" - android:layout_width="wrap_content" - android:layout_height="wrap_content" - android:text="@string/jump_to_previous" - style="@style/Widget.MaterialComponents.Button.OutlinedButton" - android:textSize="@dimen/text_medium" - android:layout_weight="1"/> + <com.google.android.material.button.MaterialButton + android:id="@+id/jumpPreviousButton" + style="@style/HierarchyButton" + android:text="@string/jump_to_previous"/>
Based on the gathered information, I can now provide a final response about the accessibility concerns:
Accessibility issues need to be addressed
Text contrast ratio is insufficient:
- Current disabled state uses
button_disabled(#d0e3ff) oncc_core_bg(#F2F2F1), which doesn't meet WCAG guidelines- Active state uses
cc_brand_color(#5D70D2) which needs verification against the backgroundButton spacing inconsistencies:
- Current padding of 12dp doesn't match the design system's
min_button_padding(20dp)- Layout weight of 1 creates equal spacing but doesn't align with standard spacers defined in dimens.xml
Missing accessibility attributes:
- Buttons lack
android:contentDescriptionfor screen readers- No state descriptions for enabled/disabled states
🔗 Analysis chain
Line range hint
39-83: Address visual accessibility concerns.Based on previous feedback, there are issues with text contrast in the disabled state and button alignment.
Please verify:
- Text contrast meets WCAG guidelines in both enabled and disabled states
- Button spacing is consistent with the Figma design
- Text capitalization follows the app's typography guidelines
Run this script to check for related accessibility styles:
Let me check the content of the color selectors and relevant styles to verify the contrast ratios and disabled states.
Let me check the button style definition and spacing guidelines.
Let me check the dimension values and specific button spacing guidelines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing button styles that might define disabled states rg -l "android:state_enabled=\"false\"" "app/res/color" rg -l "android:state_enabled=\"false\"" "app/res/drawable" # Search for text appearance styles that might define capitalization rg "android:textAllCaps" "app/res/values/styles.xml"Length of output: 422
Script:
#!/bin/bash # Check color definitions and disabled states cat app/res/color/primary_button_background.xml cat app/res/color/button_colored_text.xml cat app/res/values/colors.xml # Check the specific button style being used rg "CommCare.Button.Default" app/res/values/styles.xml -A 5Length of output: 7628
Script:
#!/bin/bash # Check button style definitions and spacing rg -A 10 "style name=\"CommCare.Button" app/res/values/styles.xml # Check if there are any specific button spacing or padding guidelines fd -e xml -e md . | xargs rg -l "button.*spacing|button.*padding|button.*margin"Length of output: 276
Script:
#!/bin/bash # Check dimension values for button spacing and padding cat app/res/values/dimens.xml cat app/res/values-w600dp/dimens.xml # Check the specific layout files for button spacing patterns cat app/res/layout/screen_form_entry.xml cat app/res/layout/action_card.xmlLength of output: 21347
app/res/drawable/commcare_actionbar_logo.xml (2)
1-5: Consider increasing the dimensions for better touch targets.The current 20dp size might be too small for action bar logos, especially on high-density screens. Material Design guidelines recommend a minimum touch target size of 24dp.
<vector xmlns:android="http://schemas.android.com/apk/res/android" - android:width="20dp" - android:height="20dp" + android:width="24dp" + android:height="24dp" android:viewportWidth="20" android:viewportHeight="20">
6-8: Consider using theme-aware colors.Instead of hardcoding
#000000, consider using a theme-aware color reference for better dark/light theme support.<path android:pathData="M10,10m-10,0a10,10 0,1 1,20 0a10,10 0,1 1,-20 0" - android:fillColor="#000000"/> + android:fillColor="?attr/colorPrimary"/>app/res/drawable/rounded_button_shape.xml (1)
2-10: Consider adding solid/stroke definitionsThe shape currently only defines the outline. Consider adding:
<solid>element for default background color<stroke>element for border if needed by the design<shape xmlns:android="http://schemas.android.com/apk/res/android" android:shape="rectangle" > + <solid android:color="@android:color/transparent" /> + <stroke + android:width="@dimen/button_stroke_width" + android:color="@color/button_stroke" /> <size android:minHeight="@dimen/min_button_height"> </size>app/res/drawable/button_background_white.xml (1)
6-7: Consider using dimension resources for stroke widthFor better maintainability and consistency, consider moving the stroke width to a dimension resource.
<stroke - android:width="2dp" + android:width="@dimen/button_stroke_width" android:color="@color/cc_brand_color" />app/res/layout/single_dialog_choice_view.xml (1)
Line range hint
11-13: Consider using dimension resources for padding valuesThe hardcoded padding values (5dp, 12dp) should be moved to dimension resources for better maintainability and consistency.
- android:drawablePadding="5dp" - android:padding="12dp" + android:drawablePadding="@dimen/icon_padding_small" + android:padding="@dimen/button_padding"app/res/layout/progress_dialog_cancel_button.xml (1)
2-3: Consider adding MaterialButton style attribute for consistencyWhile the migration to MaterialButton is correct, consider adding a style attribute to ensure consistent appearance with other buttons in the app.
<com.google.android.material.button.MaterialButton xmlns:android="http://schemas.android.com/apk/res/android" + style="@style/Widget.MaterialComponents.Button" xmlns:tools="http://schemas.android.com/tools"app/res/drawable/map_button_base_background.xml (1)
1-24: Add ripple effect for better touch feedbackThe button states are well-defined with proper theme attributes, but adding a ripple effect would enhance the touch feedback following Material Design principles.
Consider updating the selector to include ripple effect:
<?xml version="1.0" encoding="utf-8"?> -<selector xmlns:android="http://schemas.android.com/apk/res/android"> +<ripple xmlns:android="http://schemas.android.com/apk/res/android" + android:color="?attr/colorControlHighlight"> + <item android:id="@android:id/mask"> + <shape> + <corners android:radius="@dimen/rounded_button_corner_radius" /> + <solid android:color="@android:color/white"/> + </shape> + </item> <!-- Disabled state --> <item android:state_enabled="false"> <shape> <solid android:color="?attr/buttonDisabledColor" /> <corners android:radius="@dimen/rounded_button_corner_radius" /> </shape> </item> - <!-- Pressed state --> - <item android:state_pressed="true"> - <shape> - <solid android:color="?attr/buttonPressedColor" /> - <corners android:radius="@dimen/rounded_button_corner_radius" /> - </shape> - </item> <!-- Default state --> <item> <shape> <solid android:color="?attr/buttonDefaultColor" /> <corners android:radius="@dimen/rounded_button_corner_radius" /> </shape> </item> -</selector> +</ripple>app/res/layout/multiple_apps_limit_view.xml (1)
Line range hint
17-24: Extract hardcoded text and apply consistent button styleTwo suggestions for improvement:
- Move the hardcoded text "Go to App Manager" to strings.xml
- Apply the common button style for consistency across the app
Apply these changes:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" android:id="@+id/back_to_manager_button" android:layout_marginBottom="@dimen/standard_spacer_large" - android:text="Go to App Manager" + android:text="@string/go_to_app_manager" android:textSize="19sp" + style="@style/Widget.App.Button" android:layout_alignParentBottom="true" android:layout_centerHorizontal="true"/>app/res/layout/pin_screen_buttons.xml (1)
Line range hint
20-27: Add text and content description for pin_confirm_buttonThe MaterialButton needs text and content description for proper accessibility:
<com.google.android.material.button.MaterialButton style="@style/CustomButtonStyle" android:layout_width="wrap_content" android:layout_height="@dimen/min_button_height" android:id="@+id/pin_confirm_button" android:textSize="@dimen/font_size_dp_medium" android:enabled="false" - android:layout_weight="1"/> + android:layout_weight="1" + android:text="@string/confirm" + android:contentDescription="@string/confirm"/>app/res/layout/square_button_text.xml (1)
Line range hint
9-18: Convert SquareButton to MaterialButton per PR objectiveThe PR's goal is to convert all square buttons to rounded buttons, but this layout still uses
SquareButton. Consider replacing it withMaterialButtonfor consistency.- <org.commcare.views.SquareButton + <com.google.android.material.button.MaterialButton android:id="@+id/button" android:layout_width="match_parent" android:layout_height="wrap_content" - android:background="@null" android:clickable="false" - android:scaleType="fitEnd" + style="@style/Widget.CommCare.RoundedButton" />app/res/layout/draw_layout.xml (2)
Line range hint
23-27: Add missing style attributes to MaterialButtonsWhile the buttons have been converted to MaterialButton, they're missing the rounded button style attributes needed for consistency with the PR's objective.
Apply these changes to all three buttons:
<com.google.android.material.button.MaterialButton android:id="@+id/btnFinishDraw" android:layout_width="wrap_content" android:enabled="false" - android:layout_height="wrap_content"/> + android:layout_height="wrap_content" + style="@style/Widget.CommCare.RoundedButton"/>Also applies to: 35-39, 41-44
20-20: Standardize margin valuesThere are inconsistent margin declarations:
- Parent layout:
android:layout_marginHorizontal="16dp"- Reset button:
android:layout_marginHorizontal="16dp"Consider using a dimension resource for consistency:
- android:layout_marginHorizontal="16dp" + android:layout_marginHorizontal="@dimen/standard_spacing"Also applies to: 38-38
app/res/layout/entity_detail.xml (1)
14-17: Consider using MaterialButton for consistencyWhile I understand from past review comments that this button was intentionally kept as a standard Button for UI reasons, consider documenting this decision in a code comment to prevent future confusion.
<Button style="@style/ScreenPinnedButton" android:layout_width="fill_parent" android:layout_height="wrap_content" app:backgroundTint="@color/cc_attention_positive_bg" + <!-- Intentionally using standard Button instead of MaterialButton for UI consistency -->app/res/layout/layout_note_msg.xml (1)
Line range hint
45-50: Apply CustomButtonStyle for consistencyThe MaterialButton should use the established custom style for consistency with other buttons in the codebase.
Apply this diff:
<com.google.android.material.button.MaterialButton android:id="@+id/layout_note_msg_action_button" + style="@style/CustomButtonStyle" android:layout_width="match_parent" android:layout_height="wrap_content" android:text="Button" android:visibility="gone" />app/res/layout/activity_target_mismatch_error.xml (1)
Line range hint
41-47: Apply CustomButtonStyle for consistencyTo maintain consistency with other MaterialButtons in the codebase, apply the custom style.
Apply this diff:
<com.google.android.material.button.MaterialButton + style="@style/CustomButtonStyle" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_marginTop="@dimen/standard_spacer_large" android:layout_below="@+id/error_info_container" android:id="@+id/install_app_button" android:layout_centerHorizontal="true" tools:text="Install CommCare LTS"/>app/res/drawable/home_sync.xml (1)
11-11: Consider renaming color resource for consistencyThe color reference
@color/square_button_icon_colorseems inconsistent with the new rounded button theme. Consider renaming it to something more generic likebutton_icon_color.app/res/layout/blank_missing_multimedia_layout.xml (1)
Line range hint
11-14: Replace pixel-based dimensions with dp valuesUsing pixel-based dimensions (
20px,135px,663px) can lead to inconsistent sizing across different screen densities. Consider converting to dp values.-android:paddingLeft="20px" -android:layout_marginRight="20px" -android:paddingTop="20px" +android:paddingLeft="@dimen/standard_spacer" +android:layout_marginRight="@dimen/standard_spacer" +android:paddingTop="@dimen/standard_spacer"app/res/layout/privilege_claiming_view.xml (1)
Line range hint
53-58: Add style and accessibility attributes to MaterialButtonWhile the Button to MaterialButton conversion is correct, consider adding the following attributes for better consistency and accessibility:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" android:id="@+id/claim_button" android:textSize="@dimen/font_size_xlarge" - android:text="@string/claim_privilege_button_text" /> + android:text="@string/claim_privilege_button_text" + style="@style/Widget.MaterialComponents.Button" + android:contentDescription="@string/claim_privilege_button_text" />app/res/layout/activity_report_problem.xml (2)
11-11: Use dimension resource instead of hardcoded valueReplace hardcoded margin value with a dimension resource for better maintainability and consistency:
- android:layout_margin="16dp" + android:layout_margin="@dimen/standard_spacer_double"
Line range hint
50-56: Enhance MaterialButton implementationConsider the following improvements for better consistency and accessibility:
<com.google.android.material.button.MaterialButton android:id="@+id/ReportButton01" android:layout_width="match_parent" android:layout_height="wrap_content" android:layout_below="@+id/ReportText01" android:layout_centerHorizontal="true" - android:text="Submit Report"/> + android:text="Submit Report" + style="@style/Widget.MaterialComponents.Button" + android:contentDescription="@string/submit_report_description" + android:layout_marginHorizontal="@dimen/standard_spacer"/>app/res/layout/dotsdoses.xml (1)
64-69: Consider creating a centralized button style systemTo maintain consistency across the app's button styling, consider:
- Creating base button styles in
styles.xmlthat extendWidget.MaterialComponents.Button- Defining specific variants (primary, secondary, etc.) that inherit from the base style
- Creating a style guide document listing all button variants and their usage
This will help:
- Ensure visual consistency
- Simplify maintenance
- Make it easier to implement future design changes
app/res/values/attrs.xml (1)
55-57: LGTM! Consider adding documentation.The new button color attributes are well-structured and provide the necessary flexibility for button state management. Consider adding XML documentation comments to describe the intended usage of each attribute.
+ <!-- Color used for buttons in disabled state --> <attr name="buttonDisabledColor" format="reference|color"/> + <!-- Color used for buttons when pressed/clicked --> <attr name="buttonPressedColor" format="reference|color"/> + <!-- Default color for buttons in normal state --> <attr name="buttonDefaultColor" format="reference|color"/>app/res/layout/geo_progress.xml (1)
Line range hint
54-68: Add consistent Material styles for dialog buttonsThe MaterialButton conversion lacks style definitions. To maintain consistency:
- Apply appropriate Material styles that match other dialog buttons
- Consider creating a reusable style for dialog action buttons
- Ensure the button weights (1 and 1.5) create the intended visual hierarchy with MaterialButton
Consider creating a common style for dialog buttons:
<style name="Widget.App.MaterialButton.Dialog"> <item name="android:layout_margin">@dimen/standard_spacer</item> <item name="android:layout_weight">1</item> <item name="android:layout_width">0dp</item> <item name="android:layout_height">wrap_content</item> </style>app/res/layout/single_app_view.xml (1)
39-45: Add style attributes and content descriptions for accessibilityThe MaterialButton implementations look good, but a few improvements would enhance accessibility and maintain consistent styling:
- Add style attribute to maintain consistent appearance across all buttons
- Add content descriptions for screen readers
Apply these changes to each button:
<com.google.android.material.button.MaterialButton android:id="@+id/uninstall_button" android:layout_height="wrap_content" android:layout_margin="@dimen/standard_spacer" android:layout_width="fill_parent" android:onClick="rebootAlertDialog" + style="@style/Widget.MaterialComponents.Button" + android:contentDescription="@string/uninstall_app" android:text="@string/uninstall_app" />Also applies to: 47-53, 55-61, 63-70
app/res/layout/activity_mapbox_location_picker.xml (1)
67-72: Consider button hierarchy for cancel actionThe cancel button should be visually distinct from the primary action (confirm).
Consider using an outlined style:
<com.google.android.material.button.MaterialButton android:id="@+id/cancel_button" android:layout_width="fill_parent" android:layout_height="wrap_content" android:layout_gravity="bottom" + style="@style/Widget.MaterialComponents.Button.OutlinedButton" android:text="@string/cancel" />app/res/layout/install_permission_requester.xml (1)
Inconsistent button implementation found in screen_login.xml
There is one remaining standard
<Button>in screen_login.xml that needs to be migrated to MaterialButton for consistency. The rest of the codebase has properly implemented MaterialButton with consistent styling patterns.
app/res/layout/screen_login.xml: Convert<Button android:id="@+id/show_password"...>to MaterialButton🔗 Analysis chain
Line range hint
39-70: Verify consistent MaterialButton implementationLet's ensure consistent implementation of MaterialButton across the codebase.
Run this script to check for consistency:
Also applies to: 59-72, 55-62
Let me gather more information about the remaining Button implementation and MaterialButton patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining standard Buttons rg -l "<Button\s" --type xml # Check MaterialButton style patterns ast-grep --pattern 'MaterialButton { $$$ style = "$_" $$$ }' # Check MaterialButton attribute patterns ast-grep --pattern 'MaterialButton { $$$ android:(padding|margin|layout_[^=]+) = "$_" $$$ }'Length of output: 232
Script:
#!/bin/bash # Check the context of remaining Button in screen_login.xml rg -A 5 -B 5 "<Button\s" app/res/layout/screen_login.xml # Check all MaterialButton implementations for consistency rg -A 5 -B 5 "MaterialButton" --type xml # Check specific attributes on MaterialButton rg "MaterialButton.*?(android:padding|android:margin|android:layout_)" --type xmlLength of output: 59296
app/res/layout/connection_diagnostic.xml (2)
42-47: Consider standardizing button margins across the layoutThe
run_connection_testbutton has a 10dp margin, but other buttons in this layout don't have consistent margins. Consider standardizing the margin values across all buttons for better visual consistency.<com.google.android.material.button.MaterialButton android:id="@+id/run_connection_test" android:layout_width="match_parent" - android:layout_margin="10dp" + android:layout_margin="@dimen/standard_button_margin" android:layout_height="wrap_content" android:text="Run Connection Test"/>
Line range hint
61-73: Ensure proper content description for invisible buttonsThe
settings_buttonandreport_buttonare initially invisible. When they become visible, screen readers need proper context to announce these buttons appropriately.<com.google.android.material.button.MaterialButton android:id="@+id/settings_button" android:layout_width="wrap_content" android:layout_height="wrap_content" + android:contentDescription="@string/settings_button_description" android:text="Go to Settings" android:visibility="invisible"/> <com.google.android.material.button.MaterialButton android:id="@+id/report_button" android:layout_width="wrap_content" android:layout_height="wrap_content" + android:contentDescription="@string/report_button_description" android:text="Report to CommCare" android:visibility="invisible"/>app/res/layout/recording_fragment.xml (1)
Line range hint
71-84: Simplify margin declarations and ensure proper state handlingThe
action_buttonhas multiple individual margin declarations that could be simplified. Also, being initially invisible, it needs proper state handling.<com.google.android.material.button.MaterialButton android:id="@+id/action_button" android:layout_width="wrap_content" android:layout_height="wrap_content" android:padding="4dp" - android:layout_marginStart="15dp" - android:layout_marginLeft="15dp" - android:layout_marginEnd="15dp" - android:layout_marginRight="15dp" - android:layout_marginBottom="15dp" - android:layout_marginTop="15dp" + android:layout_margin="15dp" + android:contentDescription="@string/action_button_description" android:gravity="center" tools:text="Finish" android:visibility="invisible" />app/res/layout/geopoint_layout.xml (1)
Line range hint
36-57: Enhance accessibility for location-related buttonsThe location-related buttons need proper content descriptions for accessibility. Also, consider using Material Design's built-in styles for consistent button appearance.
<com.google.android.material.button.MaterialButton android:id="@+id/show_location" android:layout_width="wrap_content" android:layout_height="fill_parent" android:layout_weight="1" + android:contentDescription="@string/show_current_location_description" + style="@style/Widget.Material3.Button" android:text="@string/go_to_location" android:visibility="gone"/> <com.google.android.material.button.MaterialButton android:id="@+id/accept_location" android:layout_width="wrap_content" android:layout_height="fill_parent" android:layout_weight="1" + android:contentDescription="@string/accept_current_location_description" + style="@style/Widget.Material3.Button" android:text="@string/accept_location" /> <com.google.android.material.button.MaterialButton android:id="@+id/cancel_location" android:layout_width="wrap_content" android:layout_height="fill_parent" android:layout_weight="1" + android:contentDescription="@string/cancel_location_selection_description" + style="@style/Widget.Material3.Button" android:text="@string/cancel" />app/res/layout/fragment_setup_enter_url.xml (1)
Line range hint
68-77: Add style attribute for consistent button appearanceThe MaterialButton is missing a style attribute, which could lead to inconsistent appearance with other buttons in the app. Consider adding an appropriate style that matches the new rounded button design.
<com.google.android.material.button.MaterialButton + style="@style/PrimaryButtonStyle" android:id="@+id/start_install" android:layout_width="fill_parent" android:layout_height="wrap_content"app/res/layout/execute_recovery_measures.xml (1)
Line range hint
56-84: Add styles for consistent button appearanceAll three MaterialButtons (retry, reinstall, and select_ccz) are missing style attributes, which could lead to inconsistent appearance with other buttons in the app.
<com.google.android.material.button.MaterialButton + style="@style/PrimaryButtonStyle" android:id="@+id/retry_button" android:layout_below="@id/status_tv" .../> <com.google.android.material.button.MaterialButton + style="@style/PrimaryButtonStyle" android:id="@+id/reinstall_button" android:layout_below="@id/retry_button" .../> <com.google.android.material.button.MaterialButton + style="@style/PrimaryButtonStyle" android:id="@+id/select_ccz_button" android:layout_below="@id/reinstall_button" .../>app/res/layout/component_entity_detail_item.xml (2)
Line range hint
44-59: Add content description for phone accessibility.The phone button should have a content description that includes both the action and the phone number for better accessibility.
<com.google.android.material.button.MaterialButton android:id="@+id/detail_value_phone" android:layout_width="fill_parent" android:layout_height="wrap_content" + android:contentDescription="@string/call_phone_number" android:layout_gravity="start|center_vertical" android:layout_marginLeft="11dp"
Line range hint
149-157: Add content description for address accessibility.The address button should have a content description that includes both the action and the address for better accessibility.
<com.google.android.material.button.MaterialButton android:id="@+id/detail_address_button" android:layout_width="fill_parent" android:layout_height="wrap_content" + android:contentDescription="@string/show_address_description" android:layout_gravity="start" android:drawableLeft="@drawable/sym_action_lookup"app/res/layout/screen_form_entry.xml (1)
240-249: Consider adding content description for accessibilityThe MaterialButton implementation looks good and aligns with Material Design principles. However, consider adding
android:contentDescriptionto improve accessibility, especially since it includes an icon.<com.google.android.material.button.MaterialButton android:id="@+id/multiple_intent_dispatch_button" android:text="Batch Dispatch" android:drawableRight="@drawable/icon_chevron_right_brand" android:visibility="gone" android:layout_width="fill_parent" android:layout_height="wrap_content" android:backgroundTint="@color/cc_light_cool_accent_bg" android:textColor="@color/cc_light_cool_accent_text" + android:contentDescription="Batch dispatch button with forward arrow" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (33)
app/res/drawable-hdpi-v14/notification.pngis excluded by!**/*.pngapp/res/drawable-hdpi/icon_app_white.pngis excluded by!**/*.pngapp/res/drawable-hdpi/notification.pngis excluded by!**/*.pngapp/res/drawable-hdpi/pause_question_audio.pngis excluded by!**/*.pngapp/res/drawable-hdpi/play_question_audio.pngis excluded by!**/*.pngapp/res/drawable-ldpi-v14/notification.pngis excluded by!**/*.pngapp/res/drawable-ldpi/icon_app_white.pngis excluded by!**/*.pngapp/res/drawable-ldpi/notification.pngis excluded by!**/*.pngapp/res/drawable-mdpi-v14/notification.pngis excluded by!**/*.pngapp/res/drawable-mdpi/icon_app_white.pngis excluded by!**/*.pngapp/res/drawable-mdpi/notification.pngis excluded by!**/*.pngapp/res/drawable-mdpi/pause_question_audio.pngis excluded by!**/*.pngapp/res/drawable-mdpi/play_question_audio.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/icon_app_white.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/pause_question_audio.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/play_question_audio.pngis excluded by!**/*.pngapp/res/drawable-xxxhdpi/pause_question_audio.pngis excluded by!**/*.pngapp/res/drawable-xxxhdpi/play_question_audio.pngis excluded by!**/*.pngapp/res/drawable/ccbanner.pngis excluded by!**/*.pngapp/res/drawable/icon_app_white.pngis excluded by!**/*.pngapp/res/drawable/icon_color.pngis excluded by!**/*.pngapp/res/drawable/pause.pngis excluded by!**/*.pngapp/res/drawable/play.pngis excluded by!**/*.pngapp/res/drawable/record.pngis excluded by!**/*.pngapp/res/drawable/record_add.pngis excluded by!**/*.pngapp/res/drawable/record_in_progress.pngis excluded by!**/*.pngapp/res/drawable/record_start.pngis excluded by!**/*.pngapp/res/drawable/recording_trash.pngis excluded by!**/*.pngapp/res/mipmap-hdpi/commcare_launcher.pngis excluded by!**/*.pngapp/res/mipmap-mdpi/commcare_launcher.pngis excluded by!**/*.pngapp/res/mipmap-xhdpi/commcare_launcher.pngis excluded by!**/*.pngapp/res/mipmap-xxhdpi/commcare_launcher.pngis excluded by!**/*.pngapp/res/mipmap-xxxhdpi/commcare_launcher.pngis excluded by!**/*.png
📒 Files selected for processing (82)
app/res/color/primary_button_background.xml(1 hunks)app/res/drawable/actionbar_modern_spacing.xml(0 hunks)app/res/drawable/btn_finish_background.xml(1 hunks)app/res/drawable/button_background_white.xml(1 hunks)app/res/drawable/button_green_background.xml(0 hunks)app/res/drawable/button_orange_background.xml(0 hunks)app/res/drawable/button_red_background.xml(0 hunks)app/res/drawable/check_update.xml(1 hunks)app/res/drawable/commcare_actionbar_logo.xml(1 hunks)app/res/drawable/commcare_actionbar_logo_spacing.xml(1 hunks)app/res/drawable/home_incomplete.xml(1 hunks)app/res/drawable/home_logout.xml(1 hunks)app/res/drawable/home_saved.xml(1 hunks)app/res/drawable/home_start.xml(1 hunks)app/res/drawable/home_sync.xml(1 hunks)app/res/drawable/install_start.xml(1 hunks)app/res/drawable/install_stop.xml(1 hunks)app/res/drawable/map_button_base_background.xml(1 hunks)app/res/drawable/progressbar_modern.xml(2 hunks)app/res/drawable/progressbar_modern_blue.xml(2 hunks)app/res/drawable/rounded_button_shape.xml(1 hunks)app/res/drawable/startup_barcode.xml(1 hunks)app/res/drawable/startup_url.xml(1 hunks)app/res/drawable/wifi_direct_change_mode.xml(1 hunks)app/res/drawable/wifi_direct_discover.xml(1 hunks)app/res/drawable/wifi_direct_submit.xml(1 hunks)app/res/drawable/wifi_direct_transfer.xml(1 hunks)app/res/layout-land/activity_crash_warning.xml(1 hunks)app/res/layout/activity_crash_warning.xml(1 hunks)app/res/layout/activity_drawing_boundary.xml(1 hunks)app/res/layout/activity_mapbox_location_picker.xml(1 hunks)app/res/layout/activity_report_problem.xml(2 hunks)app/res/layout/activity_target_mismatch_error.xml(1 hunks)app/res/layout/app_manager.xml(1 hunks)app/res/layout/blank_missing_multimedia_layout.xml(2 hunks)app/res/layout/blue_outlined_button.xml(1 hunks)app/res/layout/choice_dialog_view.xml(1 hunks)app/res/layout/component_entity_detail_item.xml(3 hunks)app/res/layout/connection_diagnostic.xml(2 hunks)app/res/layout/custom_alert_dialog.xml(2 hunks)app/res/layout/dots_detail.xml(1 hunks)app/res/layout/dotsdoses.xml(1 hunks)app/res/layout/draw_layout.xml(2 hunks)app/res/layout/entity_detail.xml(2 hunks)app/res/layout/execute_recovery_measures.xml(4 hunks)app/res/layout/fragment_setup_enter_url.xml(1 hunks)app/res/layout/geo_progress.xml(1 hunks)app/res/layout/geopoint_layout.xml(1 hunks)app/res/layout/hierarchy_layout.xml(1 hunks)app/res/layout/http_request_layout.xml(1 hunks)app/res/layout/install_confirm_fragment.xml(2 hunks)app/res/layout/install_permission_requester.xml(1 hunks)app/res/layout/layout_note_msg.xml(1 hunks)app/res/layout/list_gregorian_widget.xml(6 hunks)app/res/layout/missing_multimedia_layout.xml(2 hunks)app/res/layout/multiple_apps_limit_view.xml(1 hunks)app/res/layout/pin_screen_buttons.xml(1 hunks)app/res/layout/privilege_claiming_view.xml(1 hunks)app/res/layout/progress_dialog_cancel_button.xml(1 hunks)app/res/layout/prompt_view.xml(2 hunks)app/res/layout/recording.xml(2 hunks)app/res/layout/recording_fragment.xml(1 hunks)app/res/layout/screen_form_dump.xml(2 hunks)app/res/layout/screen_form_entry.xml(2 hunks)app/res/layout/screen_login.xml(1 hunks)app/res/layout/screen_multimedia_inflater.xml(1 hunks)app/res/layout/screen_permission_request.xml(1 hunks)app/res/layout/screen_recovery.xml(2 hunks)app/res/layout/screen_seat_app.xml(1 hunks)app/res/layout/scrolling_calendar_widget.xml(1 hunks)app/res/layout/select_install_mode_fragment.xml(2 hunks)app/res/layout/single_app_view.xml(1 hunks)app/res/layout/single_dialog_choice_view.xml(1 hunks)app/res/layout/square_button_text.xml(1 hunks)app/res/layout/square_card.xml(3 hunks)app/res/layout/update_activity.xml(2 hunks)app/res/layout/user_get_available_apps.xml(1 hunks)app/res/layout/wifi_direct_main.xml(1 hunks)app/res/values/attrs.xml(1 hunks)app/res/values/colors.xml(3 hunks)app/res/values/dimens.xml(2 hunks)app/res/values/strings.xml(1 hunks)
⛔ Files not processed due to max files limit (19)
- app/res/values/styles.xml
- app/res/values/themes.xml
- app/src/org/commcare/CommCareNoficationManager.java
- app/src/org/commcare/activities/CommCareActivity.java
- app/src/org/commcare/activities/CommCareWiFiDirectActivity.java
- app/src/org/commcare/services/CommCareFirebaseMessagingService.java
- app/src/org/commcare/services/CommCareSessionService.java
- app/src/org/commcare/sync/FormSubmissionWorker.kt
- app/src/org/commcare/utils/NotificationUtil.java
- app/src/org/commcare/views/DotsHomeView.java
- app/src/org/commcare/views/dialogs/PinnedNotificationWithProgress.java
- app/src/org/commcare/views/dialogs/StandardAlertDialog.java
- app/src/org/commcare/views/widgets/AudioWidget.java
- app/src/org/commcare/views/widgets/GeoPointWidget.java
- app/src/org/commcare/views/widgets/ImageWidget.java
- app/src/org/commcare/views/widgets/IntentWidget.java
- app/src/org/commcare/views/widgets/SignatureWidget.java
- app/src/org/commcare/views/widgets/SpinnerMultiWidget.java
- app/src/org/commcare/views/widgets/VideoWidget.java
💤 Files with no reviewable changes (4)
- app/res/drawable/button_red_background.xml
- app/res/drawable/button_orange_background.xml
- app/res/drawable/actionbar_modern_spacing.xml
- app/res/drawable/button_green_background.xml
👮 Files not reviewed due to content moderation or server errors (3)
- app/res/layout/install_confirm_fragment.xml
- app/res/values/colors.xml
- app/res/layout/list_gregorian_widget.xml
🔇 Additional comments (78)
app/res/layout/screen_seat_app.xml (1)
13-15: LGTM! ImageView changes follow Android best practices.
The changes to the ImageView are well-structured:
- Using fixed dimensions (72dp) ensures consistent sizing across devices
- Using
@mipmap/commcare_launcheris the correct approach for launcher icons
app/res/drawable/commcare_actionbar_logo_spacing.xml (1)
5-5: Verify referenced drawable resource exists
The implementation references @drawable/commcare_actionbar_logo. Let's ensure this resource exists in the codebase.
✅ Verification successful
Referenced drawable exists and is properly used
The verification confirms that:
- The referenced drawable
commcare_actionbar_logo.xmlexists in the correct location - The new spacing wrapper is referenced in
themes.xmlas the action bar logo - Both files are properly located in the drawable resources directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the referenced drawable exists
# Expected: Find the referenced drawable file
# Search for the drawable resource file
fd "commcare_actionbar_logo" app/res/drawable
# Search for any references to this drawable in XML files
rg "commcare_actionbar_logo" app/res/
Length of output: 397
app/res/drawable/wifi_direct_change_mode.xml (2)
8-8: LGTM! Good practice using color resources.
The change from hardcoded color to a color resource improves maintainability and consistency.
8-8: Consider renaming the color resource to align with rounded button design.
The color resource name square_button_icon_color seems inconsistent with the PR's goal of converting to rounded buttons. Consider using a more generic name like button_icon_color to better reflect the new design direction.
Let's check if this resource is used consistently across other files:
app/res/values/strings.xml (1)
18-18: LGTM! Capitalization change follows Material Design guidelines.
The change from "Install An App" to "Install an App" follows Android's Material Design text guidelines, which recommend sentence case for UI elements.
Let's verify the string usage across the app to ensure the change is reflected correctly:
✅ Verification successful
I'll check how the string is used in the layout file to ensure it's being displayed correctly and there are no other UI elements that might need to be considered for consistency.
String resource usage verified and properly implemented
The string resource @string/install_app is correctly used in app_manager.xml as the text for a Material Button component. The layout implementation aligns well with Material Design principles, and the sentence case capitalization change is appropriate for this UI element.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for layout files using this string resource
rg -l "R.string.install_app|@string/install_app"
Length of output: 81
Script:
#!/bin/bash
# Check the usage context in app_manager.xml
cat app/res/layout/app_manager.xml
Length of output: 2368
app/res/drawable/wifi_direct_transfer.xml (2)
8-8: LGTM! Good practice using color resource reference.
The change from hardcoded color to a resource reference improves maintainability and consistency.
8-8: Consider renaming the color resource to avoid confusion.
The color resource name square_button_icon_color might be misleading since this PR's objective is to convert square buttons to rounded ones. Consider using a more neutral name like button_icon_color or primary_icon_color.
Let's check if this color resource is used consistently across other files:
app/res/layout/screen_form_dump.xml (1)
18-27: LGTM! Proper usage of density-independent pixels and RTL support.
The ImageView changes demonstrate good practices:
- Consistent use of dp units for dimensions
- Proper RTL support with Start/End attributes
- Appropriate scaling and padding configuration
app/res/drawable/wifi_direct_discover.xml (2)
8-8: LGTM! Good practice using color resources.
The change from hardcoded colors to color resources improves maintainability and theme support.
Also applies to: 11-11
8-8: Verify color resource name consistency.
The resource name square_button_icon_color seems inconsistent with the PR's goal of converting to rounded buttons. Consider renaming to a more generic name like button_icon_color.
Also applies to: 11-11
app/res/drawable/check_update.xml (1)
Line range hint 2-3: Verify if the large icon dimensions are intentional.
The vector drawable's dimensions (300dp x 232dp) seem quite large for a button icon, which could affect button sizing and layout consistency.
✅ Verification successful
The large icon dimensions are intentional and consistent with other icons in the update screen
The dimensions (300dp x 232dp) are consistent with other vector drawables used in the same update screen. Both apply_update.xml and install_stop.xml use similar dimensions (300dp x 232/233dp). These icons are used in SquareButtonWithText custom views within a horizontally arranged button group, where the layout system will handle proper scaling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for button layouts using this drawable to understand the impact
# of its dimensions on button sizing
# Search for layouts referencing this drawable
rg -l "check_update" --type xml
# Search for potential scaling attributes in button layouts
rg -A 5 'android:drawable="@drawable/check_update"' --type xml
Length of output: 171
Script:
#!/bin/bash
# Let's check the layout files to see how this drawable is used
cat app/res/layout/update_activity.xml
cat app/res/layout-land/update_activity.xml
Length of output: 14029
Script:
#!/bin/bash
# Let's check if there are any other similar vector drawables to compare dimensions
cat app/res/drawable/apply_update.xml
cat app/res/drawable/install_stop.xml
Length of output: 1439
app/res/layout/custom_alert_dialog.xml (2)
4-4: LGTM: Namespace addition is correct.
The app namespace is properly defined and necessary for Material Design attributes.
74-85: Previous review comment about inconsistent styling remains unaddressed.
The implementation still shows inconsistent styling approaches and duplicate attribute declarations.
Also applies to: 87-96, 98-111
app/res/drawable/install_stop.xml (1)
7-8: Verify icon design matches intended purpose
The path data appears to describe a circular arrow/refresh icon, but the filename suggests this should be a stop icon. This inconsistency could lead to user confusion.
✅ Verification successful
Icon design matches its intended purpose - no changes needed
The icon is correctly designed for its purpose. The circular arrow/refresh icon in install_stop.xml is appropriate as it's used in conjunction with a "Start over" action in the install confirmation UI. This is complemented by install_start.xml which shows a download/installation icon, forming a coherent set of installation-related actions. The filename "stop" refers to stopping/resetting the installation process rather than a literal stop icon.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of this drawable to understand its context
rg -l "install_stop" --type xml
# Search for any related string resources
rg "install.*stop" --type xml
Length of output: 519
Script:
#!/bin/bash
# Let's check the string resources and content around the usage to understand the context better
rg -B2 -A2 "install_stop" app/res/layout/install_confirm_fragment.xml app/res/layout/update_activity.xml
# Also search for any related drawable files that might help understand the icon system
fd "install.*\.xml" app/res/drawable/
Length of output: 1314
Script:
#!/bin/bash
# Let's check the content of install_start.xml to understand the icon system better
cat app/res/drawable/install_start.xml
# Also check for any string resources that might provide more context about the functionality
rg -B1 -A1 "Start over" --type xml
Length of output: 1674
app/res/layout/blue_outlined_button.xml (3)
2-4: LGTM! Good choice using MaterialButton
The switch to MaterialButton aligns well with the PR objective of implementing rounded buttons, as it provides built-in support for corner radius customization and modern styling options.
10-10: LGTM! Proper RTL support implemented
The layout_gravity="end" is correctly implemented, ensuring proper support for right-to-left (RTL) layouts.
5-6: Verify NegativeButtonStyle implementation
The button uses NegativeButtonStyle and brand color for stroke. Let's verify the style implementation to ensure it provides the correct rounded corners as per the Figma design.
✅ Verification successful
Style implementation verified and consistent with design requirements
The NegativeButtonStyle is correctly implemented with rounded corners and brand styling:
- Uses MaterialComponents.Button.OutlinedButton as base style
- Applies consistent corner radius via
@dimen/rounded_button_corner_radius - Properly sets rounded corners for all sides
- Correctly applies brand color for stroke
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of NegativeButtonStyle and its corner radius
# Look for style definition and any corner radius attributes
# Search for NegativeButtonStyle definition
echo "Searching for NegativeButtonStyle definition:"
rg -A 10 "NegativeButtonStyle" --type xml
# Search for cornerRadius attributes in styles
echo -e "\nSearching for cornerRadius configurations:"
rg "cornerRadius" --type xml
Length of output: 4275
app/res/drawable/startup_url.xml (2)
7-8: LGTM! Good use of color resource.
The change from hardcoded color (#FFFFFF) to @color/square_button_icon_color improves maintainability and consistency with the theme system.
✅ Verification successful
The color resource is properly defined and consistently used across multiple icon drawables
The verification confirms that:
- The color resource
square_button_icon_coloris properly defined incolors.xmlwith the value#FFFFFF - It's consistently used across multiple icon drawables in the app, including the startup URL icon
- This change aligns with the existing pattern in the codebase where this color resource is used for icon fills
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the color resource definition and its usage
# Check the color resource definition
rg "square_button_icon_color" --glob "app/res/values/colors.xml"
# Check for other usages of this color resource
rg "square_button_icon_color" --glob "app/res/**/*.xml"
Length of output: 1843
2-5: Verify the icon scaling across different screen densities.
The vector drawable has been resized from 400×325dp to 300×232dp while maintaining the aspect ratio. While vector drawables scale well, please ensure this size reduction doesn't affect visibility on different screen densities.
✅ Verification successful
Icon scaling is consistent with app-wide standards
The resized vector drawable (300×232dp) aligns with the app's design system, as evidenced by:
- Multiple icons in the app using the same 300dp width: home icons, startup icons, and installation icons
- The icon is used in a custom
SquareButtonWithTextcomponent, ensuring consistent presentation - The aspect ratio is maintained, preserving the icon's visual proportions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this icon is used in layouts with specific size constraints
# Search for references to this drawable in layout files
rg -A 3 "startup_url" --glob "app/res/layout/*.xml"
# Check for any custom size attributes applied to this drawable
rg "startup_url.*android:layout_(width|height)" --glob "app/res/layout/*.xml"
Length of output: 582
Script:
#!/bin/bash
# Check the custom view implementation for any size-related handling
ast-grep --pattern 'class SquareButtonWithText'
# Check if there are any other vector drawables with similar dimensions for consistency
rg -l "android:width=\"300dp\"" --glob "app/res/drawable/*.xml"
# Look for any dimension resources that might be related to this icon
rg "300dp|232dp" --glob "app/res/values/*.xml"
Length of output: 719
app/res/layout/missing_multimedia_layout.xml (1)
Line range hint 24-31: LGTM! Button conversion implemented correctly.
The conversion to MaterialButton aligns with the PR objective of implementing rounded buttons. The layout parameters and positioning are maintained correctly.
app/res/drawable/home_logout.xml (2)
2-5: Verify the icon dimensions match the Figma specifications.
The vector dimensions have been significantly reduced from 748dp. While this optimization is good for performance, please ensure these exact dimensions (300x233) match the Figma design specifications.
✅ Verification successful
The dimensions are consistent with other home screen icons and startup icons
The vector dimensions (300x233) are consistent with other home screen icons in the app:
home_sync.xml: 300x233home_start.xml: 300x233home_saved.xml: 300x233home_incomplete.xml: 300x233startup_barcode.xml: 300x233install_start.xml: 300x233install_stop.xml: 300x233
This appears to be the standard size for navigation icons in the app, suggesting the dimensions are intentionally aligned with the app's design system rather than being arbitrary changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these dimensions are consistent across other icon resources
rg -l "android:width=\"300dp\"" "app/res/drawable/*.xml"
rg -l "android:height=\"233dp\"" "app/res/drawable/*.xml"
Length of output: 240
Script:
#!/bin/bash
# First, let's find the correct path for drawable resources
fd -t f -e xml . | grep -i "drawable"
# Then search for vector drawables with similar dimensions
fd -t f -e xml . | grep -i "drawable" | xargs rg "android:(width|height|viewport)"
Length of output: 17624
7-8: Verify dark mode compatibility for the icon color.
The icon now uses @color/square_button_icon_color which suggests a themeable approach. Please ensure this color reference is properly defined for both light and dark themes.
app/res/layout/prompt_view.xml (3)
6-6: LGTM: Namespace addition is correct
The app namespace is properly defined for Material Design components.
40-46:
Critical: Address accessibility and styling concerns
- Accessibility issues:
- Missing content description for screen readers
- No minimum touch target size specified
- Style concerns:
- Direct use of solid_green color instead of semantic color resource
- Previous custom style was removed without clear justification
Apply these changes:
<com.google.android.material.button.MaterialButton
android:layout_height="wrap_content"
- app:backgroundTint="@color/solid_green"
+ app:backgroundTint="@color/cc_attention_positive_color"
android:textSize="@dimen/font_size_xlargest"
android:layout_margin="16dp"
android:layout_width="wrap_content"
- android:id="@+id/action_button"/>
+ android:id="@+id/action_button"
+ android:minHeight="48dp"
+ android:minWidth="48dp"
+ android:contentDescription="@string/action_button_description"
+ style="@style/Widget.Material3.Button"/>54-60:
Critical: Standardize button implementation and add accessibility support
- Accessibility issues:
- Missing content description for screen readers
- No minimum touch target size specified
- Style concerns:
- Missing style attribute
- Hardcoded padding values
Apply these changes:
<com.google.android.material.button.MaterialButton
android:layout_height="wrap_content"
android:textSize="@dimen/font_size_xlarge"
android:layout_margin="16dp"
android:layout_width="wrap_content"
- android:paddingHorizontal="24dp"
- android:id="@+id/do_later_button" />
+ android:paddingHorizontal="@dimen/standard_spacer_double"
+ android:id="@+id/do_later_button"
+ android:minHeight="48dp"
+ android:minWidth="48dp"
+ android:contentDescription="@string/do_later_button_description"
+ style="@style/Widget.Material3.Button.OutlinedButton" />Let's verify the existence of required dimensions and strings:
app/res/drawable/startup_barcode.xml (2)
7-8: Consider improving path data maintainability
While the path data correctly creates a barcode scanner icon, consider breaking it into multiple smaller paths with meaningful group IDs for better maintainability.
- <path
- android:pathData="M98.93,50.36C91.24,50.36 85,56.64 85,64.39V92.46C85,100.21 91.24,106.49 98.93,106.49H126.79C134.48,106.49 140.71,100.21 140.71,92.46V64.39C140.71,56.64 134.48,50.36 126.79,50.36H98.93ZM122.14,69.07V87.78H103.57V69.07H122.14ZM98.93,125.21C91.24,125.21 85,131.49 85,139.24V167.31C85,175.05 91.24,181.34 98.93,181.34H126.79C134.48,181.34 140.71,175.05 140.71,167.31V139.24C140.71,131.49 134.48,125.21 126.79,125.21H98.93ZM122.14,143.92V162.63H103.57V143.92H122.14ZM159.29,64.39V92.46C159.29,100.21 165.52,106.49 173.21,106.49H201.07C208.76,106.49 215,100.21 215,92.46V64.39C215,56.64 208.76,50.36 201.07,50.36H173.21C165.52,50.36 159.29,56.64 159.29,64.39ZM177.86,69.07H196.43V87.78H177.86V69.07ZM187.14,171.99V181.34H196.43V171.99H187.14ZM215,171.99H205.71V181.34H215V171.99ZM205.71,125.21V134.56H187.14V125.21H159.29V153.27V181.34H177.86V153.27H187.14V162.63H215V143.92V134.56V125.21H205.71Z"
+ <group android:id="@+id/barcode_scanner">
+ <path
+ android:id="@+id/scanner_frame_left"
+ android:pathData="M98.93,50.36C91.24,50.36 85,56.64 85,64.39V92.46C85,100.21 91.24,106.49 98.93,106.49H126.79C134.48,106.49 140.71,100.21 140.71,92.46V64.39C140.71,56.64 134.48,50.36 126.79,50.36H98.93ZM122.14,69.07V87.78H103.57V69.07H122.14Z"
+ android:fillColor="@color/square_button_icon_color"/>
+ <path
+ android:id="@+id/scanner_frame_right"
+ android:pathData="M98.93,125.21C91.24,125.21 85,131.49 85,139.24V167.31C85,175.05 91.24,181.34 98.93,181.34H126.79C134.48,181.34 140.71,175.05 140.71,167.31V139.24C140.71,131.49 134.48,125.21 126.79,125.21H98.93ZM122.14,143.92V162.63H103.57V143.92H122.14Z"
+ android:fillColor="@color/square_button_icon_color"/>
+ <!-- Add remaining paths with meaningful IDs -->
+ </group>Verify color reference consistency
Let's verify that the color reference is properly defined and used consistently across the app.
✅ Verification successful
Color reference is properly defined and consistently used
The color @color/square_button_icon_color is well-defined in colors.xml as #FFFFFF and is consistently used across multiple icon drawables in the app, including other button icons like home, install, and wifi-direct related drawables. The usage pattern suggests this is the standard color for button icons in the app's design system.
Regarding the path data suggestion, while breaking it into smaller paths would improve maintainability, it's not a critical issue as the current implementation follows the same pattern as other drawables in the app.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check color reference definition and usage
echo "Checking color definition..."
rg "name=\"square_button_icon_color\"" "app/res/values/colors.xml"
echo "\nChecking color usage..."
rg "@color/square_button_icon_color" "app/res/drawable/*.xml"
Length of output: 376
Script:
#!/bin/bash
# Let's try a more reliable approach using fd to find and search drawable files
echo "Checking color usage in drawable files..."
fd -e xml -t f . app/res/drawable --exec rg -l "@color/square_button_icon_color" {}
echo "\nChecking other potential usages of this color..."
fd -e xml -t f . app/res --exec rg -l "@color/square_button_icon_color" {}
Length of output: 1380
2-5: Verify icon dimensions consistency across the app
The new dimensions (300x233) appear to be part of a standardization effort. Let's verify that these dimensions are consistent with other icons in the application.
✅ Verification successful
Let me try a different approach to find the vector drawables in the repository.
Based on the search results, I can now provide a final response about the icon dimensions consistency:
Icon dimensions are consistent with standardization pattern
The dimensions (300x233) in startup_barcode.xml align with a clear standardization pattern across the app's home and startup icons:
- Home icons (start, sync, saved, incomplete, etc.) use 300x233
- Startup icons (barcode, url, micronode) use 300x232/233
- Other utility icons use different standardized sizes:
- Action bar icons: 24dp
- Navigation icons: 8-14dp
- App launcher icons: 108dp
The dimensions are appropriate and follow the established pattern for startup/home screen icons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in icon dimensions across vector drawables
# Look for any vector drawables with different dimensions
echo "Checking vector drawable dimensions..."
rg -l 'android:width|android:height|android:viewportWidth|android:viewportHeight' 'app/res/drawable/*.xml' | \
while read -r file; do
echo "=== $file ==="
rg 'android:(width|height|viewportWidth|viewportHeight)' "$file"
done
Length of output: 273
Script:
#!/bin/bash
# First, find all XML files in the repository
echo "Finding all XML files that might be vector drawables..."
fd -e xml | while read -r file; do
if grep -q 'vector' "$file"; then
echo "=== $file ==="
grep -E 'android:(width|height|viewportWidth|viewportHeight)' "$file" || true
fi
done
Length of output: 50113
app/res/layout/hierarchy_layout.xml (2)
52-52: LGTM: Text size consistency restored.
The addition of android:textSize="@dimen/text_medium" to jumpPreviousButton maintains consistency with other buttons in the layout.
Line range hint 39-83: Convert standard Buttons to MaterialButtons for rounded design.
To align with the PR's objective of implementing rounded buttons and following the Figma design, these buttons should be converted to MaterialButton components.
Apply this change to all three buttons:
- <Button
+ <com.google.android.material.button.MaterialButton
android:id="@+id/jumpPreviousButton"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:text="@string/jump_to_previous"
- android:padding="12dp"
- android:textColor="@color/cc_brand_color"
- android:background="@color/cc_core_bg"
- style="@style/CommCare.Button.Default"
+ style="@style/Widget.MaterialComponents.Button.OutlinedButton"
android:textSize="@dimen/text_medium"
android:layout_weight="1"/>app/res/drawable/home_start.xml (1)
2-5: Verify layout compatibility with new dimensions
The drawable dimensions have changed significantly:
- Width: 704dp → 300dp
- Height: 704dp → 233dp
- Aspect ratio changed from 1:1 to ~1.29:1
Let's check where this drawable is used to ensure layouts won't break:
✅ Verification successful
The previous search might have been too restrictive. Let's broaden it to:
- Look for any mention of "home_start" in XML files
- Look for programmatic references in code files
- Include all subdirectories under app/
This will help us find:
- Layout references using different formats (@drawable/, @+id/, etc.)
- Programmatic references (R.drawable.home_start)
- References in other resource directories (drawable-v21, drawable-hdpi, etc.)
I want to:
- See how the drawable is used in HomeButtons.java to understand if the size change would affect its usage
- Compare with other vector drawables in the same directory to see if this new size aligns with other icons
- Get more context about the implementation to make a proper assessment
Based on the analysis of the code, I can now generate the final response:
Dimension changes are safe to proceed
The drawable is only used in HomeButtons.java as part of a HomeCardDisplayData object where it's displayed in a button/card layout. The dimensions are handled appropriately:
- The drawable is used alongside other home screen icons that share the same 300dp width dimension
- The aspect ratio change is intentional as part of the UI update for rounded buttons
- The implementation in
HomeButtons.javauses a flexible card-based layout that adapts to the drawable's dimensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to home_start drawable
rg -t xml "@drawable/home_start" app/res/layout/
Length of output: 48
Script:
#!/bin/bash
# Search for references to home_start in all XML files, including different resource directories
rg -t xml "home_start" app/
# Also search for potential Java/Kotlin references
rg "R\.drawable\.home_start|@\+?id/.*home_start" app/
Length of output: 179
Script:
#!/bin/bash
# Let's check the HomeButtons.java file to understand how the drawable is used
cat app/src/org/commcare/activities/HomeButtons.java
# Also check if there are any other vector drawables in the same directory to compare dimensions
cat app/res/drawable/home_start.xml
fd -t f -e xml . app/res/drawable/ --exec head -n 6 {}
Length of output: 39472
app/res/color/primary_button_background.xml (1)
4-4: Verify color consistency with Figma design
The button's default state now uses @color/button_primary_color. Please verify this aligns with the Figma design specifications for rounded buttons.
app/res/drawable/btn_finish_background.xml (1)
7-7: Use design system color for consistency
The button's default state uses @color/start_home_button which doesn't align with the design system. Consider using @color/cc_brand_color or applying CustomButtonStyle directly.
app/res/drawable/rounded_button_shape.xml (1)
2-10: LGTM! Good use of dimension resources
The shape implementation correctly uses dimension resources for both height and corner radius, making it maintainable and consistent across the app.
app/res/drawable/wifi_direct_submit.xml (1)
8-8: Consider renaming color resource to remove "square" prefix
Since we're moving to rounded buttons throughout the app, the color resource name square_button_icon_color should be updated to something more generic like button_icon_color.
Also applies to: 11-11
app/res/layout/progress_dialog_cancel_button.xml (1)
Line range hint 2-15: Verify MaterialButton behavior when visibility changes
Since this button's visibility is controlled dynamically (currently set to "gone"), we should verify that the MaterialButton behaves correctly when shown/hidden.
✅ Verification successful
The previous search might have been too specific. Let's broaden it to find any references to this layout file or button in both Java files and XML resources. This will help us understand where and how this button is being used in the codebase.
The output shows that this button layout is included in two different progress dialog layouts. Let's examine these files to understand how the button is integrated and find the related Java classes that might control its visibility.
The layout files show that the cancel button is included in both determinate and indeterminate progress dialogs with the ID dialog_cancel_button. Let's examine the CustomProgressDialog class to understand how it manages this button's visibility.
MaterialButton visibility behavior is properly managed
The visibility of the cancel button is properly handled in CustomProgressDialog:
- Button is shown (VISIBLE) when initialized
- Becomes INVISIBLE during certain progress states
- Has proper enable/disable state management during cancellation
- Visibility changes are coordinated with other UI elements (progress bar)
The MaterialButton will behave correctly with these visibility changes as it follows standard Android View visibility patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where this button's visibility is modified
rg -l "progress_dialog_cancel_button.*visibility" --type java
Length of output: 63
Script:
#!/bin/bash
# Search for layout file references and visibility changes
rg -l "R\.layout\.progress_dialog_cancel_button|progress_dialog_cancel_button" --type java
# Also search for any usage of this layout file
rg -l "progress_dialog_cancel_button" --type xml
Length of output: 237
Script:
#!/bin/bash
# Let's examine the content of these layout files to understand how this button is included
cat app/res/layout/progress_dialog_indeterminate.xml app/res/layout/progress_dialog_determinate.xml
# Also search for any ProgressDialog related classes that might control this button
rg -l "ProgressDialog" --type java
Length of output: 5731
Script:
#!/bin/bash
# Let's examine the CustomProgressDialog class as it's likely the main controller for these dialogs
rg -A 10 "dialog_cancel_button" app/src/org/commcare/views/dialogs/CustomProgressDialog.java
# Also check for any visibility-related methods in this class
rg "setVisibility|getVisibility" app/src/org/commcare/views/dialogs/CustomProgressDialog.java
Length of output: 1379
app/res/drawable/progressbar_modern.xml (1)
5-5: Verify dimension resource existence and consistency
The change from hardcoded 4dp to @dimen/modern_progressbar_radius is good for maintainability. Let's verify:
- The dimension resource exists
- The same radius is used consistently across similar components
Also applies to: 15-15
✅ Verification successful
Let me check the content of progressbar_full.xml and submission_progressbar_drawable.xml to ensure consistent styling across all progress bars.
Let me try to view the complete content of these files to better understand their structure:
Dimension resource exists and is consistently used where needed
The verification shows that:
- The
@dimen/modern_progressbar_radiusresource is properly defined in dimens.xml with value4dp - It's consistently used in both modern-style progress bars (
progressbar_modern.xmlandprogressbar_modern_blue.xml) - The other progress bars (
progressbar_full.xmlandsubmission_progressbar_drawable.xml) intentionally don't use rounded corners as they follow a different design pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the dimension resource exists
rg "modern_progressbar_radius" --type xml
# Find all progress bar drawables that might need similar updates
fd -e xml -p "progressbar.*\.xml$"
Length of output: 810
Script:
#!/bin/bash
# Check the content of other progress bar drawables
rg -U "corners.*radius" app/res/drawable/progressbar_full.xml app/res/drawable/submission_progressbar_drawable.xml
Length of output: 116
Script:
#!/bin/bash
# View the complete content of other progress bar drawables
cat app/res/drawable/progressbar_full.xml app/res/drawable/submission_progressbar_drawable.xml
Length of output: 2094
app/res/drawable/progressbar_modern_blue.xml (2)
5-5: LGTM: Consistent with progressbar_modern.xml
The radius changes match those in progressbar_modern.xml, maintaining consistency across progress bar components.
Also applies to: 15-15
17-18: Verify color change impact and consistency
The change from start_sync_button to cc_brand_color might affect the visual appearance. Let's verify:
- The color resource exists
- Other similar components using the old color are updated
- The change aligns with the design system
✅ Verification successful
Color change is consistent with design system
The verification shows that start_sync_button was already referencing cc_brand_color in colors.xml, making this change a direct usage of the source color. The color cc_brand_color (#5D70D2) is widely used across the app as the primary brand color in themes, styles, and UI components including other progress indicators, confirming this is the correct color to use for visual consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check color resource existence and usage
rg "cc_brand_color" --type xml
# Find other components using the old color
rg "start_sync_button" --type xml
Length of output: 2648
app/res/drawable/home_saved.xml (2)
7-8: Use theme-aware color for consistent theming
The icon uses a specific color resource @color/square_button_icon_color. For better theme support and consistency:
Replace with @color/cc_core_text or define a dedicated nav_icon_color in colors.xml that can be themed appropriately.
2-5: Verify icon dimensions against Material Design guidelines
The vector drawable dimensions (300x233dp) seem unusually large for a navigation icon. Material Design typically recommends 24dp for navigation icons. These large dimensions might cause:
- Unnecessary memory overhead
- Scaling issues in different screen densities
app/res/layout/pin_screen_buttons.xml (1)
11-17: 🛠️ Refactor suggestion
Add text and content description for pin_cancel_button
The MaterialButton needs text and content description for proper accessibility:
<com.google.android.material.button.MaterialButton
style="@style/CustomButtonStyle"
android:layout_width="wrap_content"
android:layout_height="@dimen/min_button_height"
android:id="@+id/pin_cancel_button"
android:textSize="@dimen/font_size_dp_medium"
- android:layout_weight="1"/>
+ android:layout_weight="1"
+ android:text="@string/cancel"
+ android:contentDescription="@string/cancel"/>Likely invalid or redundant comment.
app/res/layout/recording.xml (2)
12-15: Add required attributes for recording control button
The same issues exist as noted in the previous review. The start recording button needs essential attributes for proper functionality and accessibility.
26-29: Add required attributes for recording control button
The same issues exist as noted in the previous review. The stop recording button needs essential attributes for proper functionality and accessibility.
app/res/drawable/install_start.xml (1)
2-8: Verify color resource availability
The vector drawable implementation looks good, using proper dimensions and following Material Design principles. Let's verify the color resource reference.
✅ Verification successful
Color resource reference is properly defined
The square_button_icon_color is correctly defined in colors.xml with a white color value (#FFFFFF), confirming proper resource availability for the vector drawable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the color resource exists and is properly defined
rg -A 1 "square_button_icon_color" app/res/values/
Length of output: 162
app/res/layout/square_button_text.xml (1)
29-30: Verify text size changes impact on button layout
The text sizes have been increased:
- Normal text: large → xlarge
- Small text: medium → large
Please verify that these larger text sizes don't cause layout issues with the new rounded button design.
✅ Verification successful
Let me gather more information about the button layout and dimensions to better understand the impact.
Let me gather one final piece of information about the button's layout constraints.
Text size changes appear safe for button layout
The increased text sizes should work well with the button layout because:
- The text view uses
wrap_contentfor height and adapts to content - It's using ResizingTextView which can handle dynamic text sizes
- The text has adequate padding (standard_spacer) on all sides
- Different screen width configurations (w320dp to w720dp) have proportionally scaled font sizes:
- xlarge: 21sp → 32sp
- large: 18sp → 28sp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of these text sizes to understand the impact
rg "font_size_(xlarge|large)" --type xml
Length of output: 2897
Script:
#!/bin/bash
# Search for layout parameters and dimensions related to the square button
rg "square_button" --type xml -A 5
# Also check for any button-related dimensions that might affect the layout
rg "button_" app/res/values*/dimens.xml
Length of output: 8547
Script:
#!/bin/bash
# Check the layout file for the square button to see its constraints and dimensions
cat app/res/layout/square_button_text.xml
Length of output: 1343
app/res/layout/screen_permission_request.xml (1)
Line range hint 36-46: Add style and accessibility attributes to permission buttons
Both permission buttons need the rounded button style and accessibility attributes for consistency.
The previous review already identified this issue. Apply the same changes to both buttons:
<com.google.android.material.button.MaterialButton
android:id="@+id/screen_permission_request_button_grant"
android:layout_width="fill_parent"
android:layout_height="wrap_content"
- android:text="Grant Access"/>
+ android:text="Grant Access"
+ android:contentDescription="@string/grant_permission_access"
+ style="@style/Widget.CommCare.RoundedButton"
+ android:layout_marginBottom="@dimen/standard_spacing"/>app/res/layout/entity_detail.xml (1)
4-4: LGTM: App namespace added for Material Design attributes
The addition of the app namespace enables the use of Material Design attributes.
app/res/layout/dots_detail.xml (2)
43-48: Apply consistent Material Design styling
The conversion to MaterialButton is good, but as mentioned in previous reviews, consider adding Material Design attributes for consistent styling.
<com.google.android.material.button.MaterialButton
android:layout_height="wrap_content"
android:layout_width="wrap_content"
android:text="Save Details"
android:id="@+id/btn_dots_detail_ok"
- android:layout_weight="0.5"/>
+ android:layout_weight="0.5"
+ style="@style/Widget.MaterialComponents.Button"
+ android:layout_margin="@dimen/standard_spacer"/>50-55: Apply consistent styling to cancel button
For visual hierarchy, consider using the outlined style for the cancel button.
<com.google.android.material.button.MaterialButton
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:id="@+id/btn_dots_detail_cancel"
android:text="Cancel"
- android:layout_weight="0.5"/>
+ android:layout_weight="0.5"
+ style="@style/Widget.MaterialComponents.Button.OutlinedButton"
+ android:layout_margin="@dimen/standard_spacer"/>app/res/drawable/home_incomplete.xml (2)
2-5: LGTM: Vector dimensions updated for better scaling
The increased dimensions and viewport size will provide better scaling across different screen sizes.
7-8: Verify icon appearance across different screen densities
The path data has been completely redesigned. While the changes look good, it's important to verify the icon's appearance across different screen densities.
✅ Verification successful
Icon redesign follows consistent design system and scale
The verification shows that:
- The icon is used in the home screen's incomplete forms button with appropriate color handling
- The path data follows similar complexity and coordinate scale (within 0-300 range) as other home icons
- All home icons use the same viewport size and consistent styling approach
The redesigned icon maintains visual consistency with the home icon family while effectively representing its "incomplete" status through clear visual elements (checkmark and alert indicator).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all layout files that reference this drawable to verify usage contexts
# Search for references to home_incomplete drawable
rg -l "(@drawable/home_incomplete|R\.drawable\.home_incomplete)" --type xml
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search to understand the usage context of this icon
# Search for any references to "home_incomplete" in all files
rg "home_incomplete"
# Also check if there are similar home-related drawables
fd "home.*xml" "app/res/drawable"
Length of output: 388
Script:
#!/bin/bash
# Let's examine the context in HomeButtons.java and check other home icons for consistency
# Check how home_incomplete is used in HomeButtons.java
rg -A 5 -B 5 "home_incomplete" app/src/org/commcare/activities/HomeButtons.java
# Look at the pathData in other home icons to compare the scale/complexity
rg "android:pathData" app/res/drawable/home_*.xml
Length of output: 10432
app/res/layout/http_request_layout.xml (1)
41-48: 🛠️ Refactor suggestion
Consider using CustomButtonStyle and content_min_margin
The codebase shows established patterns that should be followed:
- There's a
CustomButtonStyledefined that inherits fromWidget.MaterialComponents.Button content_min_margin(16dp) is the standard dimension resource for margins
Apply this diff to align with the codebase patterns:
<com.google.android.material.button.MaterialButton
android:id="@+id/request_button"
- android:layout_margin="10dp"
+ style="@style/CustomButtonStyle"
+ android:layout_margin="@dimen/content_min_margin"
android:layout_width="wrap_content"
android:layout_gravity="center"
android:layout_height="wrap_content"
android:paddingBottom="@dimen/content_start"
android:paddingTop="@dimen/content_start" />app/res/drawable/home_sync.xml (1)
2-5: Verify new dimensions match design specifications
The drawable dimensions have been significantly reduced (748dp→300dp, 664dp→233dp). Please confirm these new dimensions align with the Figma design specifications for the rounded button theme.
✅ Verification successful
Dimensions are consistent with other home icons
The new dimensions (300dp x 233dp) are consistent with all other home navigation icons in the app:
- home_incomplete.xml: 300dp x 233dp
- home_logout.xml: 300dp x 233dp
- home_saved.xml: 300dp x 233dp
- home_start.xml: 300dp x 233dp
- home_report.xml: 300dp x 232dp (negligible 1dp difference)
The reduction in size from the previous dimensions appears to be an intentional standardization across all home navigation icons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other vector drawables to compare dimensions
fd -e xml -t f . app/res/drawable/ -x grep -l "android:width.*dp"
Length of output: 1910
Script:
#!/bin/bash
# Get dimensions of home_* drawables for comparison
echo "=== home_sync.xml ==="
grep -A3 "android:width" app/res/drawable/home_sync.xml
echo -e "\n=== Other home_* drawables ==="
for file in app/res/drawable/home_*.xml; do
if [ "$file" != "app/res/drawable/home_sync.xml" ]; then
echo -e "\n$file:"
grep -A3 "android:width" "$file"
fi
done
Length of output: 4734
app/res/layout/blank_missing_multimedia_layout.xml (1)
Line range hint 25-31: LGTM: MaterialButton implementation aligns with design goals
The Button to MaterialButton conversion follows the PR objective of implementing rounded buttons.
app/res/layout/app_manager.xml (1)
39-49: LGTM: Well-structured MaterialButton implementation
The implementation:
- Correctly uses MaterialButton for rounded style
- Maintains proper spacing with dimension resources
- Preserves functionality with onClick handler
- Uses appropriate text sizing
app/res/layout/dotsdoses.xml (1)
64-69: 🛠️ Refactor suggestion
Use string resource and add accessibility support
The button implementation needs the following improvements:
<com.google.android.material.button.MaterialButton
android:layout_height="wrap_content"
android:layout_width="fill_parent"
android:id="@+id/btn_dots_doses_done"
- android:text="Done"
- android:layout_alignParentBottom="true"/>
+ android:text="@string/done_button_text"
+ android:layout_alignParentBottom="true"
+ style="@style/Widget.MaterialComponents.Button"
+ android:contentDescription="@string/done_button_description"/>Note: This comment partially duplicates previous feedback about MaterialButton styling consistency across the app.
app/res/layout/square_card.xml (2)
44-45: Text size adjustments align with material design
The text size changes follow a consistent typography scale:
- Card text: xlarge with large fallback
- Subtext: medium size
Also applies to: 57-57
31-32: Verify SquareImageView behavior with wrap_content
Using wrap_content for both dimensions of a SquareImageView might affect its square aspect ratio. Ensure this change doesn't break the intended square appearance.
✅ Verification successful
Using wrap_content is safe for SquareImageView
Looking at the SquareImageView.java implementation, the view correctly handles its dimensions in onMeasure(). It uses the width from widthMeasureSpec and calculates a proportional height based on the drawable's intrinsic dimensions, maintaining the correct aspect ratio. The wrap_content attribute will work as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SquareImageView implementation to understand dimension handling
ast-grep --pattern 'class SquareImageView {
$$$
onMeasure($$$) {
$$$
}
$$$
}'
Length of output: 91
Script:
#!/bin/bash
# Search for SquareImageView class implementation with different patterns and file extensions
rg -l "class SquareImageView"
# Search for files containing SquareImageView in their name
fd "SquareImageView"
# Search for onMeasure implementation in any custom view
rg "onMeasure.*\{" -A 10
Length of output: 9804
app/res/layout/screen_recovery.xml (1)
Line range hint 41-48: Consider adding consistent MaterialButton styling
The migration to MaterialButton aligns with the PR objective of implementing rounded buttons. However, consider adding style attributes to ensure consistent appearance across the app:
<com.google.android.material.button.MaterialButton
android:id="@+id/retry_button"
android:layout_below="@id/recovery_status"
android:layout_centerHorizontal="true"
android:layout_height="wrap_content"
android:layout_marginTop="@dimen/standard_spacer_double"
android:layout_width="wrap_content"
android:text="@string/recovery_retry"
+ style="@style/Widget.CommCare.Button"
android:visibility="gone"/>Let's verify if a common button style is defined:
Also applies to: 51-59
✅ Verification successful
Let me gather more information about the MaterialButton usage in the codebase to provide a more accurate recommendation.
Let me check one more thing to see if there's a CommCare.Button.Default style definition since it appears to be used in several layouts.
Consider using style="@style/CustomButtonStyle" for MaterialButton consistency
The codebase shows an established pattern of using MaterialButton with CustomButtonStyle for modern button implementations. This style (found in styles.xml) properly inherits from Widget.MaterialComponents.Button and includes standardized attributes for background tint, text color, size, and padding.
- Update the MaterialButton in
screen_recovery.xmlto use this style:
<com.google.android.material.button.MaterialButton
android:id="@+id/retry_button"
android:layout_below="@id/recovery_status"
android:layout_centerHorizontal="true"
android:layout_height="wrap_content"
android:layout_marginTop="@dimen/standard_spacer_double"
android:layout_width="wrap_content"
android:text="@string/recovery_retry"
+ style="@style/CustomButtonStyle"
android:visibility="gone"/>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for button style definitions
rg -A 5 'style name=".*Button"' "app/res/values/styles.xml"
Length of output: 2482
Script:
#!/bin/bash
# Search for MaterialButton usage with style attributes
rg -A 2 'MaterialButton.*style=' app/res/layout/
# Search for any existing button style references in layout files
rg -A 2 'style="@style/.*Button' app/res/layout/
Length of output: 4363
Script:
#!/bin/bash
# Search for CommCare.Button style definitions
rg "CommCare\.Button" app/res/values/
Length of output: 141
app/res/layout/screen_multimedia_inflater.xml (1)
56-63:
Style definition missing for MaterialButton
The conversion from Button to MaterialButton removed the original style (ButtonBottom). This could affect the button's appearance and behavior. Consider:
- Creating a corresponding Material style that maintains the original button's characteristics
- Verifying that the new padding values (8dp) match the Figma design specifications
app/res/layout/activity_drawing_boundary.xml (1)
Line range hint 47-77: Verify Material Design style compatibility
The conversion maintains the MapButton styles (Green/Red/Orange), but we need to ensure these styles:
- Are compatible with MaterialButton
- Preserve the original button state selectors
- Follow Material Design guidelines for colored buttons
#!/bin/bash
# Search for MapButton styles and their definitions
rg "MapButton(Red|Green|Orange)" --type xml
# Search for any selector definitions used in these styles
rg "selector.*MapButton" --type xmlThis relates to the previous discussion about button state selectors in the PR comments.
app/res/layout/install_permission_requester.xml (1)
55-62: Review padding implementation
The MaterialButton has custom padding that might conflict with the component's internal padding.
Let's verify if other permission request screens use similar padding:
Consider using MaterialButton's built-in padding or moving padding to the style:
<com.google.android.material.button.MaterialButton
android:id="@+id/get_perms_button"
android:layout_width="fill_parent"
android:layout_height="wrap_content"
android:layout_gravity="bottom"
android:nextFocusUp="@+id/edit_password"
- android:paddingBottom="@dimen/content_start"
- android:paddingTop="@dimen/content_start"
+ style="@style/Widget.MaterialComponents.Button"
+ android:insetTop="@dimen/content_start"
+ android:insetBottom="@dimen/content_start" />✅ Verification successful
Let's try a broader search to understand the padding patterns and MaterialButton usage in the codebase. This will help us verify:
- Other permission-related layouts
- How MaterialButton is used across the app
- The context of content_start dimension usage
MaterialButton padding should follow component guidelines
Looking at the codebase patterns:
- Most MaterialButtons in the app use either styles (e.g.,
CustomButtonStyle,MapButtonGreen) or standard margins/insets - The
content_start(8dp) dimension is commonly used for padding in other components but rarely for MaterialButtons - Other permission-related screens (
screen_permission_request.xml) don't apply direct padding to MaterialButtons
The review comment is correct. MaterialButton has its own internal padding, and adding direct padding attributes can lead to inconsistent appearance. Use android:insetTop and android:insetBottom for adjusting the touch target, or define a style for consistent button appearance across the app.
Example locations where MaterialButton padding is handled properly:
app/res/layout/custom_alert_dialog.xml: Uses style and standard spacingapp/res/layout/pin_screen_buttons.xml: UsesCustomButtonStyleapp/res/layout/screen_permission_request.xml: Uses default MaterialButton padding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar permission request layouts
rg -l "permission.*layout" --type xml
# Check padding patterns in MaterialButtons
ast-grep --pattern 'MaterialButton {
$$$
android:padding$_ = "$_"
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Search for layout files containing "permission" in their name or content
fd -e xml . app/res/layout | xargs rg -l "permission"
# Search for MaterialButton usage patterns in layout files
fd -e xml . app/res/layout | xargs rg -A 5 "MaterialButton"
# Search for content_start dimension usage
fd -e xml . app/res/values | xargs rg "content_start"
Length of output: 37746
app/res/layout/geopoint_layout.xml (1)
Line range hint 36-57: Verify layout behavior with weighted MaterialButtons
The horizontal layout uses weight distribution for button sizing. Verify that MaterialButtons maintain proper spacing and touch targets when weighted.
✅ Verification successful
The previous search might have failed due to pattern complexity. Let's try a broader search to understand the context better and see how MaterialButtons are typically used in layouts and if there are any established patterns for weighted buttons in horizontal layouts.
I'm focusing on a few key layout files that are likely to have similar button patterns, particularly those that might have action buttons in horizontal layouts. This will help us understand if the weight distribution pattern is consistent with other implementations in the codebase.
I'm requesting to see the complete layout files to better understand the context and implementation patterns, particularly focusing on the geopoint layout and choice dialog which commonly have horizontal button arrangements.
The weighted MaterialButton layout implementation is appropriate
The implementation in geopoint_layout.xml follows Android's recommended pattern for evenly distributed buttons in a horizontal layout:
- All buttons have
layout_width="wrap_content"withlayout_weight="1", ensuring equal distribution - The parent LinearLayout uses
fill_parentwidth with horizontal orientation - The layout includes proper padding and gravity settings
The implementation will maintain consistent button sizes and proper touch targets across different screen sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of weighted MaterialButtons to ensure consistent implementation
rg -l 'MaterialButton.*layout_weight' --type xml
# Check for potential layout issues with MaterialButtons in horizontal layouts
rg -l 'MaterialButton.*android:layout_width="wrap_content".*android:layout_weight' --type xml
Length of output: 146
Script:
#!/bin/bash
# Search for any layout files containing MaterialButton to understand the common patterns
rg -l "MaterialButton" --type xml
# Search for any horizontal LinearLayouts with weighted children to see common patterns
rg "LinearLayout.*android:orientation=\"horizontal\".*layout_weight" --type xml -A 5
Length of output: 1724
Script:
#!/bin/bash
# Let's check the specific layout files that contain MaterialButtons for weight patterns
for file in app/res/layout/geopoint_layout.xml app/res/layout/pin_screen_buttons.xml app/res/layout/screen_login.xml app/res/layout/choice_dialog_view.xml; do
echo "=== $file ==="
cat "$file" | grep -A 10 -B 2 "MaterialButton.*layout_width=\"wrap_content\""
done
# Also check for any horizontal LinearLayouts with multiple buttons
for file in app/res/layout/geopoint_layout.xml app/res/layout/pin_screen_buttons.xml app/res/layout/screen_login.xml app/res/layout/choice_dialog_view.xml; do
echo "=== $file ==="
cat "$file" | grep -A 15 -B 2 "LinearLayout.*orientation=\"horizontal\".*>.*MaterialButton"
done
Length of output: 2929
Script:
#!/bin/bash
# Let's try a simpler approach to view the entire geopoint_layout.xml file
echo "=== geopoint_layout.xml ==="
cat app/res/layout/geopoint_layout.xml
# And check a few other layout files that might have similar button patterns
echo "=== choice_dialog_view.xml ==="
cat app/res/layout/choice_dialog_view.xml
Length of output: 6347
app/res/layout/choice_dialog_view.xml (1)
28-39: Verify dialog button design implementation
Based on the previous discussion in the comments, dialog buttons should maintain their original square design rather than using rounded corners. The current implementation with NegativeButtonStyle might need to be reconsidered.
Please confirm if this dialog button should use a different style that maintains square corners, as shown in the previously approved design:
app/res/layout/execute_recovery_measures.xml (1)
20-22: Inconsistent image dimensions detected
The commcare_launcher image dimensions (136dp x 136dp) are inconsistent with other layouts that use 72dp x 72dp.
app/res/layout/activity_crash_warning.xml (1)
111-116: 🛠️ Refactor suggestion
Maintain consistency with landscape layout
The MaterialButton implementation should match the attributes added to the landscape layout.
Apply the same changes as suggested for the landscape layout.
app/res/layout/scrolling_calendar_widget.xml (1)
Line range hint 137-144: Enhance calendar button implementation
The MaterialButton implementation needs improvements for accessibility and consistency:
Apply this diff to enhance the button:
<com.google.android.material.button.MaterialButton
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:text="Today"
android:id="@+id/today"
android:layout_centerHorizontal="true"
android:layout_marginTop="@dimen/standard_spacer"
- android:layout_marginBottom="@dimen/standard_spacer"/>
+ android:layout_marginBottom="@dimen/standard_spacer"
+ style="@style/CustomButtonStyle"
+ android:text="@string/calendar_today_button"
+ android:contentDescription="@string/calendar_today_button_description"/>Additionally, ensure that the calendar navigation is fully accessible by:
- Adding appropriate content descriptions to navigation buttons
- Ensuring proper focus order for keyboard navigation
app/res/values/dimens.xml (2)
108-108: LGTM: Modern progressbar radius
The 4dp radius for the modern progressbar provides a subtle rounded corner, which aligns well with Material Design principles.
27-28: Consider reducing the rounded_button_corner_radius value
The current radius of 22dp seems excessive and might affect the button's usable area. Material Design guidelines typically recommend more subtle rounded corners for buttons (4-8dp).
Consider reducing the radius to align with Material Design standards:
- <dimen name="rounded_button_corner_radius">22dp</dimen>
+ <dimen name="rounded_button_corner_radius">8dp</dimen>app/res/layout/wifi_direct_main.xml (1)
101-101: 🛠️ Refactor suggestion
Verify button height behavior in different screen sizes
Changing from wrap_content to match_parent in a weighted layout might cause inconsistent button sizes across different screen sizes. Consider using a fixed height or wrap_content with minHeight for more predictable sizing.
Consider this alternative approach:
- android:layout_height="match_parent"
+ android:layout_height="wrap_content"
+ android:minHeight="@dimen/min_button_height"Also applies to: 113-113, 125-125, 137-137
app/res/layout/update_activity.xml (1)
98-98: LGTM: Consistent color scheme application
The color changes align with the new button styling system and maintain consistency with other screens.
Also applies to: 107-107
app/res/layout/component_entity_detail_item.xml (1)
Line range hint 108-113: LGTM!
The MaterialButton implementation for the callout button is correct and maintains all necessary attributes.
app/res/layout/select_install_mode_fragment.xml (1)
75-78: LGTM! Color updates align with design system.
The color changes for both buttons (barcode_button_color and url_button_color) maintain visual consistency while preserving the distinct purpose of each button.
Also applies to: 95-98
app/res/layout/user_get_available_apps.xml (2)
142-142: LGTM! Padding update maintains consistent spacing.
The horizontal padding update to @dimen/standard_spacer_large maintains consistent spacing across the UI.
137-142: 🛠️ Refactor suggestion
Add missing content description for accessibility.
While the MaterialButton implementation aligns with the PR objective of updating to rounded buttons, it's missing an important accessibility attribute.
app/res/layout/screen_form_entry.xml (1)
59-59: Verify the semantic meaning of the background color
The navigation pane's background color has been changed to @color/barcode_button_color, which seems semantically disconnected from its purpose. Consider using a more appropriately named color resource that reflects the navigation pane's function.
app/res/layout/screen_login.xml (2)
169-176: Verify button sizing after padding removal
While the conversion to MaterialButton is good, the removal of padding attributes might affect the button's vertical sizing. Consider adding explicit padding or using a style to maintain consistent button dimensions across the app.
<com.google.android.material.button.MaterialButton
android:id="@+id/login_button"
android:layout_width="fill_parent"
android:layout_height="wrap_content"
android:layout_gravity="bottom"
android:textAllCaps="true"
android:nextFocusUp="@+id/edit_password"
+ android:padding="@dimen/standard_spacer"
+ style="@style/Widget.MaterialComponents.Button"
/>Line range hint 200-205: Verify the appearance of notifications button
The removal of custom attributes (backgroundColor, img, and textColor) from RectangleButtonWithText might affect its appearance. Please ensure that these styles are being applied through an alternative mechanism or consider converting this to a MaterialButton for consistency.
| <com.google.android.material.button.MaterialButton | ||
| app:strokeColor="@color/cc_brand_color" | ||
| android:id="@+id/positive_button" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="@dimen/min_button_height" | ||
| android:padding="@dimen/standard_spacer" | ||
| android:layout_alignParentRight="true" | ||
| android:layout_marginRight="@dimen/standard_spacer" | ||
| android:background="@android:color/transparent" | ||
| android:textSize="@dimen/font_size_dp_medium" | ||
| android:textColor="@color/dialog_button_color" | ||
| android:layout_marginLeft="@dimen/standard_spacer" | ||
| android:visibility="gone" | ||
| android:layout_alignParentEnd="true" | ||
| android:layout_marginEnd="@dimen/standard_spacer"> | ||
| </Button> | ||
| android:layout_marginEnd="@dimen/standard_spacer"/> |
There was a problem hiding this comment.
Improve RTL support with consistent attribute usage.
The buttons mix Right/Left with Start/End attributes which can cause layout issues in RTL languages. Choose one approach consistently:
- Modern approach (recommended): Use Start/End
- Legacy approach: Use Left/Right
Apply this diff to standardize RTL attributes:
<com.google.android.material.button.MaterialButton
style="@style/CustomButtonStyle"
android:id="@+id/positive_button"
android:layout_width="wrap_content"
android:layout_height="@dimen/min_button_height"
android:padding="@dimen/standard_spacer"
- android:layout_alignParentRight="true"
android:layout_alignParentEnd="true"
- android:layout_marginRight="@dimen/standard_spacer"
- android:layout_marginLeft="@dimen/standard_spacer"
android:layout_marginStart="@dimen/standard_spacer"
android:layout_marginEnd="@dimen/standard_spacer"
android:visibility="gone"/>
<com.google.android.material.button.MaterialButton
style="@style/CustomButtonStyle"
android:id="@+id/neutral_button"
android:layout_width="wrap_content"
android:layout_height="@dimen/min_button_height"
android:padding="@dimen/standard_spacer"
- android:layout_marginRight="@dimen/standard_spacer"
android:layout_marginEnd="@dimen/standard_spacer"
- android:layout_toLeftOf="@id/positive_button"
android:layout_toStartOf="@id/positive_button"
android:visibility="gone"/>
<com.google.android.material.button.MaterialButton
style="@style/CustomButtonStyle"
android:id="@+id/negative_button"
android:layout_width="wrap_content"
android:layout_height="@dimen/min_button_height"
android:padding="@dimen/standard_spacer"
- android:layout_toLeftOf="@id/neutral_button"
android:layout_toStartOf="@id/neutral_button"
- android:layout_marginRight="@dimen/standard_spacer"
- android:layout_marginLeft="@dimen/standard_spacer"
android:layout_marginStart="@dimen/standard_spacer"
android:layout_marginEnd="@dimen/standard_spacer"
android:visibility="gone"/>Also applies to: 87-96, 98-111
| <clip-path | ||
| android:pathData="M72,38h156v156h-156z"/> |
There was a problem hiding this comment.
Review clip-path for rounded button compatibility
The square clip-path (M72,38h156v156h-156z) might prevent rounded corners from showing. Consider using a rounded rectangle clip-path to match the PR's rounded button requirements.
Consider updating to a rounded rectangle clip-path:
- android:pathData="M72,38h156v156h-156z"/>
+ android:pathData="M72,38 h156 a8,8 0 0 1 8,8 v140 a8,8 0 0 1 -8,8 h-140 a8,8 0 0 1 -8,-8 v-140 a8,8 0 0 1 8,-8 z"/>📝 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.
| <clip-path | |
| android:pathData="M72,38h156v156h-156z"/> | |
| <clip-path | |
| android:pathData="M72,38 h156 a8,8 0 0 1 8,8 v140 a8,8 0 0 1 -8,8 h-140 a8,8 0 0 1 -8,-8 v-140 a8,8 0 0 1 8,-8 z"/> |
| <path | ||
| android:pathData="M215.69,44.61C209.02,37.94 198.23,37.94 191.56,44.61L182.39,53.75L212.22,83.58L221.39,74.41C228.06,67.74 228.06,56.95 221.39,50.28L215.69,44.61ZM124.53,111.64C122.67,113.5 121.24,115.79 120.42,118.32L111.4,145.37C110.51,147.99 111.21,150.89 113.16,152.87C115.11,154.85 118.01,155.52 120.66,154.63L147.71,145.62C150.21,144.76 152.5,143.36 154.39,141.5L205.36,90.5L175.5,60.64L124.53,111.64V111.64ZM101.25,57.5C85.1,57.5 72,70.6 72,86.75V164.75C72,180.9 85.1,194 101.25,194H179.25C195.4,194 208.5,180.9 208.5,164.75V135.5C208.5,130.11 204.14,125.75 198.75,125.75C193.36,125.75 189,130.11 189,135.5V164.75C189,170.14 184.64,174.5 179.25,174.5H101.25C95.86,174.5 91.5,170.14 91.5,164.75V86.75C91.5,81.36 95.86,77 101.25,77H130.5C135.89,77 140.25,72.64 140.25,67.25C140.25,61.86 135.89,57.5 130.5,57.5H101.25Z" | ||
| android:fillColor="@color/square_button_icon_color"/> |
There was a problem hiding this comment.
💡 Codebase verification
Update color resource name and its references for consistency with rounded design
The color resource square_button_icon_color is widely used across multiple icon drawables in the app. Since we're moving to a rounded button design system, we should update both the color resource name and all its references.
colors.xml: Update the color resource name- Update references in 17 drawable files:
install_start.xmlwifi_direct_transfer.xmlwifi_direct_discover.xmlwifi_direct_change_mode.xmlstartup_url.xmlstartup_barcode.xmlwifi_direct_submit.xmlhome_start.xmlhome_logout.xmlhome_sync.xmlcheck_update.xmlhome_incomplete.xmlhome_saved.xmlinstall_stop.xml
-<color name="square_button_icon_color">#FFFFFF</color>
+<color name="button_icon_color">#FFFFFF</color>And update all references from @color/square_button_icon_color to @color/button_icon_color.
🔗 Analysis chain
Update color resource name for consistency
The color resource @color/square_button_icon_color should be renamed to align with the new rounded button design system.
Consider updating the color resource name:
- android:fillColor="@color/square_button_icon_color"/>
+ android:fillColor="@color/button_icon_color"/>Don't forget to update the color resource name in colors.xml as well.
Let's check for other usages of this color:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the color resource
rg "square_button_icon_color" app/res/
Length of output: 1678
| <path | ||
| android:pathData="M17.591,8.821C17.286,7.737 16.584,6.836 15.611,6.287C15.111,6.005 14.57,5.834 14.018,5.775C13.992,5.539 13.95,5.304 13.886,5.071C13.582,3.987 12.879,3.086 11.907,2.537C10.934,1.987 9.809,1.854 8.738,2.162C7.667,2.47 6.777,3.181 6.235,4.166C5.956,4.672 5.788,5.22 5.729,5.779C5.496,5.805 5.264,5.847 5.034,5.912C3.963,6.22 3.073,6.931 2.53,7.916C1.988,8.9 1.856,10.039 2.16,11.124C2.465,12.208 3.167,13.108 4.14,13.658C4.639,13.94 5.181,14.11 5.733,14.17C5.759,14.405 5.801,14.641 5.865,14.873C6.169,15.958 6.872,16.858 7.844,17.408C8.817,17.957 9.942,18.09 11.013,17.782C12.084,17.474 12.974,16.763 13.516,15.778C13.795,15.273 13.963,14.725 14.022,14.165C14.255,14.139 14.487,14.097 14.717,14.032C15.788,13.724 16.678,13.013 17.221,12.029C17.762,11.044 17.893,9.905 17.591,8.821ZM7.498,4.88C7.853,4.238 8.432,3.773 9.132,3.572C9.832,3.371 10.566,3.458 11.201,3.816C11.837,4.175 12.295,4.762 12.494,5.47C12.532,5.606 12.557,5.743 12.574,5.882C12.634,6.365 12.569,6.852 12.378,7.303C12.341,7.391 12.3,7.479 12.252,7.565C12.145,7.76 11.948,8.091 11.711,8.481C10.755,7.868 8.835,6.644 8.203,6.287C7.875,6.102 7.53,5.969 7.176,5.879C7.217,5.532 7.326,5.194 7.498,4.88ZM10.946,9.973C10.946,10.571 10.466,11.057 9.875,11.057C9.284,11.057 8.804,10.571 8.804,9.973C8.804,9.374 9.284,8.889 9.875,8.889C10.466,8.889 10.946,9.374 10.946,9.973ZM6.233,11.666C6.05,11.997 5.919,12.347 5.831,12.705C5.488,12.662 5.154,12.552 4.844,12.377C4.21,12.019 3.75,11.432 3.551,10.723C3.353,10.015 3.438,9.272 3.793,8.629C4.147,7.985 4.727,7.521 5.426,7.32C5.561,7.281 5.696,7.257 5.833,7.239C6.31,7.179 6.792,7.244 7.237,7.437C7.324,7.475 7.411,7.518 7.496,7.566C7.688,7.674 8.015,7.874 8.401,8.114C7.796,9.082 6.587,11.027 6.233,11.666ZM12.251,15.064C11.897,15.706 11.317,16.172 10.617,16.373C9.918,16.574 9.184,16.487 8.548,16.128C7.913,15.77 7.454,15.183 7.256,14.475C7.217,14.339 7.193,14.201 7.176,14.063C7.116,13.58 7.18,13.093 7.37,12.642C7.407,12.553 7.448,12.465 7.497,12.38C7.604,12.185 7.801,11.854 8.038,11.463C8.995,12.076 10.914,13.301 11.547,13.658C11.874,13.843 12.22,13.976 12.574,14.066C12.532,14.412 12.424,14.751 12.251,15.064ZM15.955,11.314C15.601,11.958 15.021,12.422 14.322,12.623C14.188,12.662 14.052,12.686 13.915,12.704C13.438,12.764 12.957,12.699 12.511,12.506C12.424,12.468 12.337,12.426 12.252,12.377C12.06,12.269 11.733,12.069 11.347,11.829C11.953,10.861 13.162,8.918 13.515,8.277C13.698,7.946 13.829,7.596 13.918,7.238C14.26,7.281 14.594,7.391 14.904,7.566C15.538,7.925 15.998,8.511 16.197,9.22C16.395,9.928 16.31,10.673 15.955,11.314Z" | ||
| android:fillColor="#ffffff"/> |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add accessibility description and ensure theme compatibility
The logo needs accessibility support and theme compatibility improvements:
- Add a new string resource for the logo's content description since none exists:
<string name="commcare_logo_description">CommCare Logo</string>- Use theme-aware colors for both paths:
<vector xmlns:android="http://schemas.android.com/apk/res/android"
+ android:contentDescription="@string/commcare_logo_description"
android:width="20dp"
android:height="20dp"
android:viewportWidth="20"
android:viewportHeight="20">
<path
android:pathData="M10,10m-10,0a10,10 0,1 1,20 0a10,10 0,1 1,-20 0"
- android:fillColor="#000000"/>
+ android:fillColor="?attr/colorPrimary"/>
<path
android:pathData="M17.591,8.821C17.286,7.737..."
- android:fillColor="#ffffff"/>
+ android:fillColor="?attr/colorOnPrimary"/>The codebase shows usage of theme attributes (e.g., ?attr/colorControlNormal in ic_tts.xml), confirming this is the correct approach for theme-aware colors.
🔗 Analysis chain
Add accessibility description and improve theme compatibility.
The logo needs accessibility support and better theme compatibility:
- Add a content description for screen readers
- Use theme-aware colors for the foreground
<vector xmlns:android="http://schemas.android.com/apk/res/android"
+ android:contentDescription="@string/commcare_logo"
android:width="20dp"
android:height="20dp"
android:viewportWidth="20"
android:viewportHeight="20">
<path
android:pathData="M10,10m-10,0a10,10 0,1 1,20 0a10,10 0,1 1,-20 0"
android:fillColor="?attr/colorPrimary"/>
<path
android:pathData="M17.591,8.821C17.286,7.737 16.584,6.836 15.611,6.287C15.111,6.005 14.57,5.834 14.018,5.775C13.992,5.539 13.95,5.304 13.886,5.071C13.582,3.987 12.879,3.086 11.907,2.537C10.934,1.987 9.809,1.854 8.738,2.162C7.667,2.47 6.777,3.181 6.235,4.166C5.956,4.672 5.788,5.22 5.729,5.779C5.496,5.805 5.264,5.847 5.034,5.912C3.963,6.22 3.073,6.931 2.53,7.916C1.988,8.9 1.856,10.039 2.16,11.124C2.465,12.208 3.167,13.108 4.14,13.658C4.639,13.94 5.181,14.11 5.733,14.17C5.759,14.405 5.801,14.641 5.865,14.873C6.169,15.958 6.872,16.858 7.844,17.408C8.817,17.957 9.942,18.09 11.013,17.782C12.084,17.474 12.974,16.763 13.516,15.778C13.795,15.273 13.963,14.725 14.022,14.165C14.255,14.139 14.487,14.097 14.717,14.032C15.788,13.724 16.678,13.013 17.221,12.029C17.762,11.044 17.893,9.905 17.591,8.821ZM7.498,4.88C7.853,4.238 8.432,3.773 9.132,3.572C9.832,3.371 10.566,3.458 11.201,3.816C11.837,4.175 12.295,4.762 12.494,5.47C12.532,5.606 12.557,5.743 12.574,5.882C12.634,6.365 12.569,6.852 12.378,7.303C12.341,7.391 12.3,7.479 12.252,7.565C12.145,7.76 11.948,8.091 11.711,8.481C10.755,7.868 8.835,6.644 8.203,6.287C7.875,6.102 7.53,5.969 7.176,5.879C7.217,5.532 7.326,5.194 7.498,4.88ZM10.946,9.973C10.946,10.571 10.466,11.057 9.875,11.057C9.284,11.057 8.804,10.571 8.804,9.973C8.804,9.374 9.284,8.889 9.875,8.889C10.466,8.889 10.946,9.374 10.946,9.973ZM6.233,11.666C6.05,11.997 5.919,12.347 5.831,12.705C5.488,12.662 5.154,12.552 4.844,12.377C4.21,12.019 3.75,11.432 3.551,10.723C3.353,10.015 3.438,9.272 3.793,8.629C4.147,7.985 4.727,7.521 5.426,7.32C5.561,7.281 5.696,7.257 5.833,7.239C6.31,7.179 6.792,7.244 7.237,7.437C7.324,7.475 7.411,7.518 7.496,7.566C7.688,7.674 8.015,7.874 8.401,8.114C7.796,9.082 6.587,11.027 6.233,11.666ZM12.251,15.064C11.897,15.706 11.317,16.172 10.617,16.373C9.918,16.574 9.184,16.487 8.548,16.128C7.913,15.77 7.454,15.183 7.256,14.475C7.217,14.339 7.193,14.201 7.176,14.063C7.116,13.58 7.18,13.093 7.37,12.642C7.407,12.553 7.448,12.465 7.497,12.38C7.604,12.185 7.801,11.854 8.038,11.463C8.995,12.076 10.914,13.301 11.547,13.658C11.874,13.843 12.22,13.976 12.574,14.066C12.532,14.412 12.424,14.751 12.251,15.064ZM15.955,11.314C15.601,11.958 15.021,12.422 14.322,12.623C14.188,12.662 14.052,12.686 13.915,12.704C13.438,12.764 12.957,12.699 12.511,12.506C12.424,12.468 12.337,12.426 12.252,12.377C12.06,12.269 11.733,12.069 11.347,11.829C11.953,10.861 13.162,8.918 13.515,8.277C13.698,7.946 13.829,7.596 13.918,7.238C14.26,7.281 14.594,7.391 14.904,7.566C15.538,7.925 15.998,8.511 16.197,9.22C16.395,9.928 16.31,10.673 15.955,11.314Z"
- android:fillColor="#ffffff"/>
+ android:fillColor="?attr/colorOnPrimary"/>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the string resource exists and check for similar logos
# that might need the same updates
# Check if the content description string resource exists
rg "commcare_logo" -t xml
# Find similar vector drawables that might need the same updates
fd -e xml -x grep -l "android:fillColor=\"#000000\"" {} \; -x grep -l "android:fillColor=\"#ffffff\"" {} \;
Length of output: 682
Script:
#!/bin/bash
# Let's check for string resources containing "logo" to find potential content description
rg "logo" -t xml app/res/values/
# Also check for theme-aware color attributes usage in drawables
rg "attr/color" -t xml app/res/drawable/
Length of output: 940
| <?xml version="1.0" encoding="utf-8"?> | ||
| <shape xmlns:android="http://schemas.android.com/apk/res/android"> | ||
| <solid android:color="@color/white" /> <!-- Background color --> | ||
| <corners android:radius="22dp" /> <!-- Rounded corners --> |
There was a problem hiding this comment.
💡 Codebase verification
Corner radius of 22dp is inconsistent with app's button styling
Based on the search results, most rounded elements in the app use a 12dp radius (seen in multiple files like blue_tabbed_box.xml, grey_bordered_box.xml, info_bubble.xml, bubble.xml). The 22dp radius in button_background_white.xml is significantly larger and inconsistent with this pattern. Only border_dashed_edges.xml uses a similar larger radius of 18dp, but it's not a button element.
- Consider adjusting the corner radius to 12dp to maintain consistency with other UI elements
- If a different radius is required by design, document the reason for deviation
🔗 Analysis chain
Verify corner radius consistency with Figma design
The corner radius of 22dp should be verified against the Figma design specifications to ensure consistency across all rounded buttons in the app.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other corner radius definitions in drawable files
rg -l "android:radius" --type xml
Length of output: 869
Script:
#!/bin/bash
# Search for corner radius values in drawable files
rg "android:radius=\"\d+dp\"" --type xml -A 1
Length of output: 3306
Script:
#!/bin/bash
# Search for button-specific drawables with corner radius
rg "android:radius=\"\d+dp\"" --type xml -A 2 -B 2 "*button*.xml"
Length of output: 118
Script:
#!/bin/bash
# Search for button-specific drawables with corner radius, using full paths
rg "android:radius=\"\d+dp\"" --type xml -A 2 -B 2 "button_background_white.xml|rounded_button_shape.xml|map_button_base_background.xml"
Length of output: 260
| @@ -1,11 +1,12 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Button xmlns:android="http://schemas.android.com/apk/res/android" | |||
| <Button | |||
There was a problem hiding this comment.
💡 Codebase verification
This Button should be converted to MaterialButton for consistency
The investigation reveals:
- The
selector_button_pressdrawable only handles press states with transparent backgrounds and doesn't provide rounded corners - The same file (
screen_form_entry.xml) already uses MaterialButton, showing inconsistency in button styling - No comments or documentation found explaining the "exit form UI" exception
Action items:
- Convert this Button to MaterialButton to maintain consistency with the PR's objective
- If there's a specific reason for keeping it as a regular Button, document it clearly in the code with a comment
- Update the styling to match the new rounded button design
🔗 Analysis chain
Verify if this Button should be converted to MaterialButton
While there's a note about "exit form UI" being the reason for not changing this button, we should:
- Document this exception clearly in the code
- Verify if this exception is still necessary
- Ensure the current styling matches the new rounded button design
The current background (selector_button_press) might not provide rounded corners consistent with other buttons in the app.
Also applies to: 9-13
Let me check the implementation of selector_button_press to understand the styling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of selector_button_press
rg -l "selector_button_press" --type xml
# Check for MaterialButton usage across layout files
ast-grep --pattern 'com.google.android.material.button.MaterialButton'
Length of output: 193
Script:
#!/bin/bash
# Check the implementation of selector_button_press drawable
fd selector_button_press.xml --type f --exec cat {}
# Check for other MaterialButton usage in these specific files
rg "MaterialButton" app/res/layout/screen_form_entry.xml app/res/layout/single_dialog_choice_view.xml
# Look for any comments about exit form UI in these files
rg -B2 -A2 "exit" app/res/layout/screen_form_entry.xml app/res/layout/single_dialog_choice_view.xml
Length of output: 956
| <com.google.android.material.button.MaterialButton | ||
| android:id="@+id/confirm_location_button" | ||
| android:layout_width="fill_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="bottom" | ||
| android:enabled="false" | ||
| style="@style/Commcare.Button.Primary" | ||
| android:text="@string/confirm_location" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add visual feedback for disabled state
The confirm location button is disabled by default, but there's no clear visual indication of this state.
Add style and state list animator:
<com.google.android.material.button.MaterialButton
android:id="@+id/confirm_location_button"
android:layout_width="fill_parent"
android:layout_height="wrap_content"
android:layout_gravity="bottom"
android:enabled="false"
+ style="@style/Widget.MaterialComponents.Button"
+ android:stateListAnimator="@null"
android:text="@string/confirm_location" />📝 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.
| <com.google.android.material.button.MaterialButton | |
| android:id="@+id/confirm_location_button" | |
| android:layout_width="fill_parent" | |
| android:layout_height="wrap_content" | |
| android:layout_gravity="bottom" | |
| android:enabled="false" | |
| style="@style/Commcare.Button.Primary" | |
| android:text="@string/confirm_location" /> | |
| <com.google.android.material.button.MaterialButton | |
| android:id="@+id/confirm_location_button" | |
| android:layout_width="fill_parent" | |
| android:layout_height="wrap_content" | |
| android:layout_gravity="bottom" | |
| android:enabled="false" | |
| style="@style/Widget.MaterialComponents.Button" | |
| android:stateListAnimator="@null" | |
| android:text="@string/confirm_location" /> |
| <com.google.android.material.button.MaterialButton | ||
| android:id="@+id/RestartCommCare" | ||
| android:layout_width="fill_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_alignParentBottom="true" | ||
| android:layout_margin="@dimen/content_min_margin" | ||
| android:background="@color/cc_brand_color" | ||
| android:textColor="@color/cc_neutral_bg"/> | ||
| android:layout_margin="@dimen/content_min_margin"/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing attributes for MaterialButton
The MaterialButton implementation needs the following improvements:
- Missing text attribute for the restart action
- No style attribute for UI consistency
Apply this diff to enhance the button implementation:
<com.google.android.material.button.MaterialButton
android:id="@+id/RestartCommCare"
android:layout_width="fill_parent"
android:layout_height="wrap_content"
android:layout_alignParentBottom="true"
- android:layout_margin="@dimen/content_min_margin"/>
+ android:layout_margin="@dimen/content_min_margin"
+ style="@style/CustomButtonStyle"
+ android:text="@string/restart_commcare"
+ android:contentDescription="@string/restart_commcare_description"/>Committable suggestion skipped: line range outside the PR's diff.
| android:layout_margin="4dp" | ||
| android:layout_weight="1" | ||
| android:padding="@dimen/content_min_margin" | ||
| SquareButtonWithText:backgroundColor="@color/cc_attention_positive_color" | ||
| SquareButtonWithText:backgroundColor="@color/start_home_button" |
There was a problem hiding this comment.
💡 Codebase verification
Color contrast ratios need adjustment for better accessibility
Based on the color values found:
start_home_button(#6AB849) with white text - contrast ratio 2.95:1start_save_button(#1B998B) with white text - contrast ratio 3.52:1cc_dark_warm_accent_color(#FEAF31) with white text - contrast ratio 1.96:1cc_brand_color(#5D70D2) with white text - contrast ratio 2.83:1
None of these combinations meet the WCAG 2.0 minimum contrast ratio requirement of 4.5:1 for normal text or 3:1 for large text. Suggested adjustments:
- Darken the button colors, or
- Use dark text (#000000) instead of white (#FFFFFF)
🔗 Analysis chain
Verify color contrast ratios for accessibility
The new button colors should maintain sufficient contrast ratios with the text color (white) to ensure accessibility.
Consider implementing a color system that ensures WCAG 2.0 compliance for all button colors.
Also applies to: 117-117, 129-129, 141-141
Let me gather more information about the text colors used with these buttons.
Let me get the white color definition to verify the contrast ratios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for color definitions
rg -A 1 "name=\"start_home_button|start_save_button|cc_dark_warm_accent_color|cc_brand_color\"" --type xml
Length of output: 4053
Script:
#!/bin/bash
# Search for SquareButtonWithText style definitions and text color attributes
rg -A 5 "style.*SquareButtonWithText|textColor.*SquareButtonWithText" --type xml
# Also search for any default text colors in styles
rg "android:textColor=\"#|textColor=\"@color" --type xml
Length of output: 7102
Script:
#!/bin/bash
# Search for white color definition
rg "name=\"white\"" --type xml -A 1
Length of output: 185
…ed_button # Conflicts: # app/res/values/styles.xml
-- remove bold from title bar -- change of button bg colour
This reverts commit edbc979.
|
@shubham1g5 @pm-dimagi I believe we can merge this. I did a pass around the aspects that |
|
@avazirna sounds good to me |
|
@damagatchi retest this please |
This PR is related to the design changes and it contains the update of the button all over the app now the square button is converted to the rounded button
Feature Flag
https://dimagi.atlassian.net/browse/CCCT-536
Release Note
All the buttons in the commcare side is converted to the rounded button as per the figma design
link to the figma is attached [here](url)
Product Description
Converted all the buttons to the rounded buttons
Doc for the screenshot reference and requested changes
https://docs.google.com/document/d/1xkqPrLClFrUONOa6gIOk1408NE1S23Wjy-cCefIGdUI/edit?usp=sharing
test.1.mp4
test.2.mp4
test.3.mp4
test.4.mp4
PR Checklist
Automated test coverage
Safety story
I have tested the one of the forms with app code 48PZ1dJ
I have tested Login flow
I have tested settings page
I have tested menu options
QA Note: All buttons during normal testing should be carefully examined to observe any anamolies or anything that doesn't seem to fit well in overall UX of the screen.
Release Note: Change in Button Styling.