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
[TIMOB-23645] Android: Added Color to Android notification color behind icon #9444
Conversation
…nd icon via setColor Native Method - Added Documentation
# Conflicts: # android/modules/android/src/java/ti/modules/titanium/android/notificationmanager/NotificationProxy.java
} | ||
setProperty(TiC.PROPERTY_COLOR, color); | ||
} | ||
|
||
@Kroll.method @Kroll.setProperty |
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.
Please check the indentation. I fixed it previously but you want to check this as well - maybe for future PR's 😙.
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.
CR passed. Awaiting another peer-review from @ypbnv.
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.
Check comments.
@@ -71,6 +72,9 @@ public void handleCreationDict(KrollDict d) | |||
if (d.containsKey(TiC.PROPERTY_LARGE_ICON)) { | |||
setLargeIcon(d.get(TiC.PROPERTY_LARGE_ICON)); | |||
} | |||
if (d.containsKey(TiC.PROPERTY_COLOR)) { |
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.
Fix indentation.
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.
Done
@@ -183,7 +187,16 @@ public void setLargeIcon(Object icon) | |||
} | |||
setProperty(TiC.PROPERTY_LARGE_ICON, icon); | |||
} | |||
|
|||
|
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.
Remove spaces from empty lines - here and on line 199.
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.
Done
@Kroll.method @Kroll.setProperty | ||
public void setColor(String color) | ||
{ | ||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP)) { |
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.
Please, import android.os.Build
One extra bracket is added after the if
statement.
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.
Done
- removed spaces - imported android.os.Build
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.
CR: Pass
|
||
See Android Reference for the [color](https://developer.android.com/reference/android/app/Notification.html#color) property. | ||
type: String | ||
since: "7.0.0" |
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.
Also add osver: {android: {min: "5.0"}}
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.
Done
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.
@garymathews - I see this has not been approved yet?
Generated by 🚫 dangerJS |
FR Passed. Notification shows the specified Studio Ver: 4.10.0.201709271713 |
@sgtcoolguy , can you please merge this PR. Its a community PR, probably unit tests can't be added. |
JIRA: https://jira.appcelerator.org/browse/TIMOB-23645