-
Notifications
You must be signed in to change notification settings - Fork 78
task(SDK-1556) - Minor improvements around dot sep #815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
task(SDK-1556) - Minor improvements around dot sep #815
Conversation
WalkthroughA new constant for alternate message summary was added to the constants file. The template renderer was updated to support this alternate summary value during timer notification rendering. Resource handling in the content view was streamlined, and the default color in notification styles now references a constant instead of a hardcoded value. Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/ContentView.kt (1)
50-52
: Simplified dot separator handling, but verify the utility method behavior.The changes streamline the dot separator implementation by using a direct drawable reference. However, there are a few concerns:
- The result of
Utils.setBitMapColour
is no longer assigned torenderer.pt_dot
- verify this is intentional- Catching only
NullPointerException
instead of a broaderException
might miss other potential errors during bitmap processingConsider this approach if the utility method behavior has changed:
private fun setDotSep() { try { renderer.pt_dot = R.drawable.pt_dot_sep - Utils.setBitMapColour(context, renderer.pt_dot, renderer.pt_meta_clr, PTConstants.PT_META_CLR_DEFAULTS) - } catch (_: NullPointerException) { + Utils.setBitMapColour(context, renderer.pt_dot, renderer.pt_meta_clr, PTConstants.PT_META_CLR_DEFAULTS) + } catch (e: Exception) { - PTLog.debug("NPE while setting dot sep color") + PTLog.debug("Error while setting dot sep color: ${e.message}") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/PTConstants.java
(1 hunks)clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateRenderer.kt
(3 hunks)clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/content/ContentView.kt
(1 hunks)clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/styles/Style.kt
(2 hunks)
🔇 Additional comments (6)
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/PTConstants.java (1)
121-121
: LGTM! Well-structured constant addition.The new
PT_MSG_SUMMARY_ALT
constant follows the established naming convention and is appropriately positioned with other alternate message constants. This supports the enhanced timer notification functionality.clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/styles/Style.kt (2)
11-11
: Good practice using centralized constants.Adding the PTConstants import supports the use of centralized constants instead of hardcoded values.
43-43
:❓ Verification inconclusive
Verify the impact of changing default icon color from white to grey.
This change modifies the default small icon color from white (
#FFFFFF
) to grey (#A6A6A6
). This could significantly impact notification visibility, especially on dark backgrounds where white icons are more prominent than grey ones.Please verify that this color change aligns with design requirements and doesn't negatively affect user experience.
🏁 Script executed:
#!/bin/bash # Description: Search for references to the old hardcoded color to ensure consistency # Expected: No remaining hardcoded "#FFFFFF" references for notification icons rg -i "#FFFFFF" --type kotlin --type java -A 2 -B 2Length of output: 54070
Verify consistency of white→grey default icon color change across all styles
The default small icon color in push templates was updated from white (
#FFFFFF
) to grey (#A6A6A6
). A search for remaining#FFFFFF
usages revealed numerous references in sample apps, core style defaults, and tests. Please confirm whether this change should be propagated (or exceptions kept) and update any dependent code or documentation accordingly:• sample/src/main/java/com/clevertap/demo/ui/main/HomeScreenViewModel.kt
–unselectedTabColor = "#FFFFFF"
–navBarColor = "#FFFFFF"
• clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java
–String WHITE = "#FFFFFF";
• clevertap-core/src/main/java/com/clevertap/android/sdk/CTInboxStyleConfig.java
–this.navBarColor = "#FFFFFF";
–this.tabBackgroundColor = "#FFFFFF";
• Test fixtures and unit tests
–clevertap-core/src/test/java/.../InAppFixtures.kt
(HTML/CSS backgrounds using#ffffff
)
–CTInboxListViewFragmentTest.kt
(JSONbg":"#ffffff"
)
–CTLocalInAppTest.kt
(radius
,bg
or text colors set to#FFFFFF
)
–CTnAppNotificationTest.kt
(assertEquals("#FFFFFF", notification.backgroundColor)
)Next steps:
- Confirm with design whether push‐template icons should now default to grey only, or if white remains expected elsewhere.
- Update sample code, style constants, fixtures, and tests to reflect the intended default.
- Verify notification visibility and contrast in all affected modules and sample apps.
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/TemplateRenderer.kt (3)
70-70
: Good addition following established patterns.The new
pt_msg_summary_alt
property declaration is consistent with other alternate value properties likept_title_alt
andpt_msg_alt
.
421-421
: Proper initialization using the new constant.The initialization correctly uses the
PT_MSG_SUMMARY_ALT
constant and follows the same pattern as other alternate properties.
285-290
: Well-implemented timer notification enhancement.The implementation correctly handles the alternate message summary during timer notification rendering. The null and empty checks are appropriate, and the conditional logic for JSON vs Bundle updates matches the existing pattern for other alternate values.
35ced64
into
task/SDK-1556/replace_multi_layouts_with_dimens
* task(SDK-1556) - Deletes all v19 layouts since minSDKVersion is 21 * task(SDK-1556) - Deletes files in v22 and v23 that are same as v21 * task(SDK-1556) - Deletes files in v22 that are same as v21 * task(SDK-1556) - Deletes files in v31 that are same as default folder * task(SDK-1556) - Fixes manual_carousel in default folder and deletes redundant file in v31 * task(SDK-4057) - Makes v21 layouts as the default layouts Also removes redundant attributes like marginLeft which were needed for v17 * task(SDK-4057) - Replaces redundant layout files with different dimensions * task(SDK-4057) - Replaces for five cta, redundant layout files with different dimensions * task(SDK-4057) - Introduces pt_render_terminal to render the final notification - Some refactoring - Introduces key * task(SDK-4536) - Fixes negative value for timer template - Changes support for timer template to Android O and above - Updates docs * task(SDK-4465) - Adds prefix pt_ to res files of push-templates to avoid conflicts - Also removes redundant attributes * task(SDK-4465) - Changes marginLeft to marginStart - Changes marginRight to marginEnd * Task/sdk 4595/replicate standard and basic (#788) * task(SDK-4595) - Replicate Standard Android Notification with Basic Push Template - Changes in dimens - Removes redundant styles.xml for sw-400 * task(SDK-4595) - Fixes color * task(SDK-1556) - Makes timer margin and padding same as other templates * fix(SDK-1556) - fixes zero bezel on API 31 and above * Task/sdk 4048/ctas pt (#786) * POC for FG service based timer notif * task(SDK-4048) - Adds CTAs in the remote view for push templates below API level 31 * task(SDK-4048) - Reverts rating view changes * Rename .java to .kt * task(SDK-4048) - Adds action buttons to remote view - For API 31 and above StyleWithActionButtons is responsible - For API 30 and below ActionButtonsContentView is responsible * task(SDK-4048) - Resolves comments - Makes classes internal - Removes redundant colour attribute - Adds pt_ prefix * task(SDK-4048) - Resolves comments - Removes return@forEach * task(SDK-4048) - Resolves comments - Adds extension function to increase readability * task(SDK-4048) - Resolves comments - Removes public access modifier * task(SDK-4048) - Testing - Adds UTs for TemplateRenderer * task(SDK-4537) - Removes extra space from manual_carousel.xml (#794) - Removes padding from shape and adds margin * task(SDK-4048) - Removes dependency from core-sdk * task(SDK-4040) - Adds CTA for manual carousel * task(SDK-4040) - Adds CTA for auto carousel * task(SDK-4048) - Compresses forward and backward arrows for manual carousel * task(SDK-4048) - Separates PendingIntent from ActionButton.kt * task(SDK-4048) - Uses composition instead of inheritance for action buttons * task(SDK-4048) - Fixes image in rating template * task(SDK-4048) - Fixes string resource name * task(SDK-4048) - Fixes rating template padding * task(SDK-4048) - Minor PR comments * task(SDK-4655) - Dark mode for push templates (#795) * task(SDK-4655) - Dark mode for push templates - Adds dark mode support for push templates - Removes bg as mandatory key - Parses colour based on dark mode of phone * task(SDK-4655) - Addresses few todos * task(SDK-4655) - Makes product display action colour optional * task(SDK-4655) - Addresses todos * task(SDK-4655) -Fixes typo and deletes redundant files * task(SDK-4655) - Color for CTA in clicked state * task(SDK-4655) - Fixes default colour for title * task(SDK-4655) - Fixes default colour for small icon * task(SDK-4655) - Adds color for rating confirm button for light and dark mode * task(SDK-4655) - Fixes default colour for product template buttons * task(SDK-4655) - Extracts dimens * task(SDK-4655) - Improves code reuse by refactoring setting of colours * task(SDK-4655) - Improves code reuse by refactoring dark mode colors * task(SDK-4834) - Makes image in notification configurable for scaletype (#808) * task(SDK-4834) - Makes image in notification configurable for scaletype * task(SDK-4834) - Improves code reuse by refactoring dark mode colors * task(SDK-4834) - Minor PR comments * task(SDK-1556) - Fixes tests * Tests/sdk 1556/pt unit tests (#811) * task(SDK-1556) - Adds test for getFlipInterval * task(SDK-1556) - Adds test for getColour * task(SDK-1556) - Adds test for createColourMap * task(SDK-1556) - Adds test for getFallback, getTimerEnd, getApplicationName, getTimeStamp, loadImageRidIntoRemoteView * task(SDK-1556) - Adds test for getImageListFromExtras, getCTAListFromExtras, getDeepLinkListFromExtras, getBigTextFromExtras, getSmallTextFromExtras, getPriceFromExtras * task(SDK-1556) - Adds test for convertRatingBundleObjectToHashMap, getEventNameFromExtras * task(SDK-1556) - Adds test for getEventPropertiesFromExtras * task(SDK-1556) - Adds test * task(SDK-1556) - Adds test for raiseNotificationClicked, getActionKeys * task(SDK-1556) - Adds test for raiseNotificationClicked * task(SDK-1556) - Adds test for deleteImageFromStorage * task(SDK-1556) - Adds test for isNotificationChannelEnabled * task(SDK-1556) - Adds test * task(SDK-1556) - Adds test for getTimerThreshold * task(SDK-1556) - Adds test for fromJson * task(SDK-1556) - Adds test for loadImageUrLIntoRemoteView * tests(SDK-1556) - Improves timer template test * tests(SDK-1556) - Improves timer template test * task(SDK-1556) - Improves tests and Utils function * task(SDK-1556) - Moves mockkStatic inside a code-block * task(SDK-1556) - Minor improvements around dot sep (#815) * task(SDK-1556) - Minor improvements around dot sep * task(SDK-1556) - Minor colour improvement * task(SDK-1556) - Minor improvements around message summary for timer template * task(SDK-1556) - Coderabbit pr comments * task(SDK-1556) - Fixes flaky test * docs(SDK-1556) - CHANGELOG and version upgrade for PT (#814) * docs(SDK-1556) - CHANGELOG and version upgrade for PT * task(SDK-1556) - Improves docs and updates dates * task(SDK-1556) - copyTemplates * docs(SDK-1556) - Improves changelog
Summary by CodeRabbit