Task/sdk 1556/replace multi layouts with dimens #734
Conversation
…redundant file in v31
WalkthroughThis pull request involves a comprehensive refactoring of layout resources in the CleverTap push templates library. The changes include removing several layout files across different Android version-specific directories (v19, v21, v22, v23, v31), updating existing layouts with more flexible dimension references, and introducing new dimension resources. The modifications aim to improve layout consistency, support right-to-left layouts, and enhance resource management through centralized dimension definitions. Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Also removes redundant attributes like marginLeft which were needed for v17
…ifferent dimensions
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
clevertap-pushtemplates/src/main/res/layout/cv_small_zero_bezel.xml (1)
Critical: Layout removal will break existing template implementation
The layout file
cv_small_zero_bezel.xmlis actively used byZeroBezelMixedSmallContentView.ktwhich is not being removed or modified. Removing this layout file without proper migration will cause runtime crashes when the zero bezel template is used.
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/ZeroBezelMixedSmallContentView.ktneeds to be updated to use the new layout system or removed as well.🔗 Analysis chain
Line range hint
1-62: Verify the impact of removing this layout file.According to the AI summary, this file is being removed as part of a broader restructuring effort. Please ensure that:
- The functionality provided by this layout is properly migrated to the new dimension-based system
- All references to this layout in the codebase are updated accordingly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to this layout file in the codebase rg -l "cv_small_zero_bezel" --type xml --type java --type kotlinLength of output: 180
Script:
#!/bin/bash # Check the content and status of the Kotlin file cat clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/ZeroBezelMixedSmallContentView.kt # Check if this file is being modified/removed in the current changes git status clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/ZeroBezelMixedSmallContentView.ktLength of output: 1322
🧹 Nitpick comments (7)
clevertap-pushtemplates/src/main/res/layout/content_view_small_single_line_msg.xml (2)
7-10: Consider using paddingStart/paddingEnd for better RTL support.While the standardization of padding dimensions is good, consider using
paddingStart/paddingEndinstead ofpaddingLeft/paddingRightfor better right-to-left (RTL) language support.- android:paddingLeft="@dimen/padding_horizontal" - android:paddingRight="@dimen/padding_horizontal" + android:paddingStart="@dimen/padding_horizontal" + android:paddingEnd="@dimen/padding_horizontal"
Line range hint
1-60: Consider optimizing layout hierarchy.The layout uses nested RelativeLayouts which can impact performance. Consider:
- Flattening the hierarchy by moving the inner RelativeLayout's children to the root
- Using ConstraintLayout for better performance and easier maintenance
Would you like me to provide an optimized version using ConstraintLayout?
clevertap-pushtemplates/src/main/res/layout/content_view_small_multi_line_msg.xml (2)
Line range hint
27-34: Consider enhancing text appearance handling.While the RTL support and margin changes look good, consider extracting text-related attributes (maxLines, ellipsize) into a style to improve maintainability.
- android:maxLines="1" - android:ellipsize="end" - android:textAppearance="@style/PushTitle" + android:textAppearance="@style/PushTitle.SingleLine"Then define in styles.xml:
<style name="PushTitle.SingleLine"> <item name="android:maxLines">1</item> <item name="android:ellipsize">end</item> </style>
Line range hint
1-58: Well-structured layout with good RTL support.The layout changes effectively support the PR's goal of consolidating layouts and improving RTL support. The consistent use of dimension resources and proper RTL attributes makes the layout more maintainable and globally applicable.
Consider creating a comprehensive design system document that outlines:
- The dimension hierarchy (@dimen/padding_* values)
- Text appearance styles and their use cases
- Layout patterns for push notifications
This will help maintain consistency as the template system grows.clevertap-pushtemplates/src/main/res/values/dimens.xml (1)
24-24: LGTM! Consider adding documentation for the new dimensions.The addition of these dimension resources aligns well with the goal of centralizing layout dimensions. This will make future maintenance easier.
Consider adding comments to document the purpose and usage of these new dimensions:
+ <!-- Maximum width for subtitle text in metadata layout --> <dimen name="metadata_subtitle_max_width">80dp</dimen> + <!-- Padding for five CTA layout elements --> <dimen name="five_cta_padding">6dp</dimen> + <!-- Top margin for product display carousel layout --> <dimen name="product_display_margin_top">8dp</dimen>Also applies to: 34-34, 43-43
clevertap-pushtemplates/src/main/res/layout/five_cta_collapsed.xml (1)
6-6: LGTM! Consider enhancing RTL support.The change to use a dimension resource improves maintainability.
Consider adding start/end padding attributes for better RTL support:
- android:padding="@dimen/five_cta_padding" + android:paddingStart="@dimen/five_cta_padding" + android:paddingEnd="@dimen/five_cta_padding" + android:paddingTop="@dimen/five_cta_padding" + android:paddingBottom="@dimen/five_cta_padding"clevertap-pushtemplates/src/main/res/layout/cv_small_text_only.xml (1)
17-17: Consider using consistent dimension resources for margins.While the change to use padding_micro is good, consider creating specific margin dimensions for better semantic meaning.
+ <!-- In dimens.xml --> + <dimen name="content_margin_end">3dp</dimen> + <dimen name="title_margin_top">3dp</dimen> - android:layout_marginEnd="@dimen/padding_micro" + android:layout_marginEnd="@dimen/content_margin_end" - android:layout_marginTop="@dimen/padding_micro" + android:layout_marginTop="@dimen/title_margin_top"Also applies to: 42-42
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
clevertap-pushtemplates/src/main/res/layout-v19/content_view_small_multi_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v19/content_view_small_single_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v19/cv_small_text_only.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v19/cv_small_zero_bezel.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v19/metadata.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v21/content_view_small_multi_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v21/content_view_small_single_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v21/cv_small_text_only.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v21/cv_small_zero_bezel.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v21/metadata.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v22/content_view_small_multi_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v22/content_view_small_single_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v22/cv_small_text_only.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v22/cv_small_zero_bezel.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v22/metadata.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v23/content_view_small_multi_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v23/content_view_small_single_line_msg.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v23/cv_small_text_only.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v23/cv_small_zero_bezel.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v23/metadata.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v31/auto_carousel.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v31/five_cta_collapsed.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v31/five_cta_expanded.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v31/image_only_big.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v31/manual_carousel.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout-v31/product_display_template.xml(0 hunks)clevertap-pushtemplates/src/main/res/layout/content_view_small_multi_line_msg.xml(2 hunks)clevertap-pushtemplates/src/main/res/layout/content_view_small_single_line_msg.xml(3 hunks)clevertap-pushtemplates/src/main/res/layout/cv_small_text_only.xml(3 hunks)clevertap-pushtemplates/src/main/res/layout/cv_small_zero_bezel.xml(2 hunks)clevertap-pushtemplates/src/main/res/layout/five_cta_collapsed.xml(1 hunks)clevertap-pushtemplates/src/main/res/layout/five_cta_expanded.xml(1 hunks)clevertap-pushtemplates/src/main/res/layout/manual_carousel.xml(2 hunks)clevertap-pushtemplates/src/main/res/layout/metadata.xml(1 hunks)clevertap-pushtemplates/src/main/res/layout/product_display_template.xml(1 hunks)clevertap-pushtemplates/src/main/res/values-v23/dimens.xml(1 hunks)clevertap-pushtemplates/src/main/res/values-v31/dimens.xml(1 hunks)clevertap-pushtemplates/src/main/res/values/dimens.xml(3 hunks)
💤 Files with no reviewable changes (26)
- clevertap-pushtemplates/src/main/res/layout-v31/auto_carousel.xml
- clevertap-pushtemplates/src/main/res/layout-v31/five_cta_collapsed.xml
- clevertap-pushtemplates/src/main/res/layout-v31/five_cta_expanded.xml
- clevertap-pushtemplates/src/main/res/layout-v21/metadata.xml
- clevertap-pushtemplates/src/main/res/layout-v31/manual_carousel.xml
- clevertap-pushtemplates/src/main/res/layout-v23/cv_small_text_only.xml
- clevertap-pushtemplates/src/main/res/layout-v23/content_view_small_single_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v22/cv_small_text_only.xml
- clevertap-pushtemplates/src/main/res/layout-v21/content_view_small_single_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v23/metadata.xml
- clevertap-pushtemplates/src/main/res/layout-v19/content_view_small_multi_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v22/content_view_small_single_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v22/metadata.xml
- clevertap-pushtemplates/src/main/res/layout-v23/content_view_small_multi_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v22/content_view_small_multi_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v21/content_view_small_multi_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v22/cv_small_zero_bezel.xml
- clevertap-pushtemplates/src/main/res/layout-v21/cv_small_zero_bezel.xml
- clevertap-pushtemplates/src/main/res/layout-v21/cv_small_text_only.xml
- clevertap-pushtemplates/src/main/res/layout-v19/content_view_small_single_line_msg.xml
- clevertap-pushtemplates/src/main/res/layout-v19/metadata.xml
- clevertap-pushtemplates/src/main/res/layout-v31/product_display_template.xml
- clevertap-pushtemplates/src/main/res/layout-v19/cv_small_zero_bezel.xml
- clevertap-pushtemplates/src/main/res/layout-v31/image_only_big.xml
- clevertap-pushtemplates/src/main/res/layout-v23/cv_small_zero_bezel.xml
- clevertap-pushtemplates/src/main/res/layout-v19/cv_small_text_only.xml
✅ Files skipped from review due to trivial changes (4)
- clevertap-pushtemplates/src/main/res/layout/five_cta_expanded.xml
- clevertap-pushtemplates/src/main/res/values-v23/dimens.xml
- clevertap-pushtemplates/src/main/res/values-v31/dimens.xml
- clevertap-pushtemplates/src/main/res/layout/manual_carousel.xml
🔇 Additional comments (8)
clevertap-pushtemplates/src/main/res/layout/cv_small_zero_bezel.xml (2)
28-29: Consider standardizing dimension resource usage.The changes replace various padding/margin values with
@dimen/padding_micro. While this simplifies the dimension system, consider whether using the same dimension for different spacing contexts (vertical padding vs top margin) is the intended design.Also applies to: 41-41
Line range hint
20-23: LGTM: Proper RTL support implementation.The layout correctly implements RTL support by:
- Using both
paddingLeft/paddingRightwith their correspondingpaddingStart/paddingEnd- Using
layout_alignStartinstead oflayout_alignLeftfor text alignmentAlso applies to: 37-38, 51-52
clevertap-pushtemplates/src/main/res/layout/content_view_small_single_line_msg.xml (1)
29-29: LGTM! Good use of standard dimensions.The margin changes align well with the dimension standardization effort, and the existing RTL support is maintained.
clevertap-pushtemplates/src/main/res/layout/content_view_small_multi_line_msg.xml (2)
7-10: LGTM! Padding values follow Material Design guidelines.The standardization of padding values using separate dimensions for horizontal and vertical padding improves visual hierarchy and maintains consistency.
Line range hint
49-56: Verify image scaling behavior.While the RTL support is properly implemented, the
centerCropscale type might cause image distortion for certain aspect ratios. Consider iffitCenterwould be more appropriate for your use case, or add content description for accessibility.✅ Verification successful
centerCropis the correct choice for notification iconsThe
centerCropscale type is consistently used across all similar notification layouts in the codebase to ensure square icons maintain a clean, consistent appearance without empty spaces. UsingfitCenterwould create visual inconsistencies in the notification UI.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any image resources specifically designed for this layout fd -e png -e jpg -e webp . clevertap-pushtemplates/src/main/res/drawable* -x file {}Length of output: 375
Script:
#!/bin/bash # Check the complete layout file and similar layouts echo "=== Current layout file ===" cat clevertap-pushtemplates/src/main/res/layout/content_view_small_multi_line_msg.xml echo -e "\n=== Similar layout files ===" rg -l "large_icon" --type xml echo -e "\n=== Other ImageViews with scaleType ===" rg "android:scaleType" --type xml -B 1 -A 1Length of output: 23369
clevertap-pushtemplates/src/main/res/layout/cv_small_text_only.xml (1)
8-10: LGTM! Consistent use of padding_micro dimension.The standardization of padding values improves consistency.
clevertap-pushtemplates/src/main/res/layout/metadata.xml (1)
41-41: Verify the impact of reduced subtitle width.The maxWidth has been reduced from 120dp to 80dp, which could affect text truncation.
✅ Verification successful
UI adjustment is safe to proceed with
The reduced maxWidth (80dp) is safe as Android's TextView will handle text truncation automatically. The subtitle text is properly processed for HTML content and uses the platform's native overflow handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for subtitle text length usage in the codebase rg -l "subtitle" --type java | xargs rg "setText.*subtitle" -B 2 -A 2Length of output: 2023
Script:
#!/bin/bash # Search for pt_subtitle initialization and length checks rg "pt_subtitle" --type java -B 2 -A 2 # Search for UI tests related to subtitle fd -e java -e kt | grep -i "test" | xargs rg "subtitle" -B 2 -A 2Length of output: 5154
clevertap-pushtemplates/src/main/res/layout/product_display_template.xml (1)
17-17: LGTM! Good use of dimension resource.The change improves maintainability while preserving the original spacing.
| <ImageView | ||
| android:id="@+id/large_icon" | ||
| android:layout_width="@dimen/large_icon" | ||
| android:layout_height="@dimen/large_icon" | ||
| android:layout_marginStart="@dimen/metadata_title_margin_horizontal" | ||
| android:layout_alignParentEnd="true" | ||
| android:scaleType="centerCrop"/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content description for accessibility.
The ImageView is missing a content description, which is important for accessibility. Consider adding android:contentDescription or android:importantForAccessibility="no" if the image is decorative.
<ImageView
android:id="@+id/large_icon"
android:layout_width="@dimen/large_icon"
android:layout_height="@dimen/large_icon"
android:layout_marginStart="@dimen/metadata_title_margin_horizontal"
android:layout_alignParentEnd="true"
- android:scaleType="centerCrop"/>
+ android:scaleType="centerCrop"
+ android:contentDescription="@string/notification_large_icon"/>Committable suggestion skipped: line range outside the PR's diff.
Details here
https://wizrocket.atlassian.net/browse/SDK-1556
It will be simpler to review this PR commitwise
Summary by CodeRabbit
Refactor
Style
Chores
dimens.xmlfiles