-
Notifications
You must be signed in to change notification settings - Fork 226
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
feat(android): Support android call notification style #1134
base: main
Are you sure you want to change the base?
feat(android): Support android call notification style #1134
Conversation
8f906ab
to
1bc815f
Compare
Just to note I see these commits flying by in my notification queue and I'm excited to see this when you think it's ready |
😮 I can remove the draft PR; I thought the draft would not spam, sorry
Yeah, I struggled since on 1.10, I had some duplicate deps errors and needed to update the rooms deps too. So yeah, I'm doing the minimal since it is not the focus, as you mentioned! While I have your attention, I struggle with the |
No worries about spam, it's a drop in the bucket and it's pleasing to see notificaitons about a feature PR vs yet another "I can't compile NNN because of ABC" issue comment 😅
I'm a fan of "tested and working" over perfect, so I lean towards something that you know works first. Second, I'm not sure what exactly would be better, passing it down the way it is now doesn't seem so bad The only thing I can see that makes me 🤔 is the inconsistent use of constants vs "magic numbers and strings" - I love the enums at the TS level and it would be nice to see constants or enums to match at the java level (and in the TS warning, which uses the numbers again vs the call types) In general though, looks like it is shaping up. At first I thought this was just duplication of our full screen intent style (which is targeted for this use case as well) but as soon as I looked at the APIs in use it looks like this is just me being unaware of the problem area and that the APIs have moved on (as they always seem to for notifications...) to have specific call notification type for modern android. Nice to support it |
That's wonderful! |
Without room update we had errors like below ``` Duplicate class kotlin.collections.jdk8.CollectionsJDK8Kt found in modules kotlin-stdlib-1.8.0 ```
1f9e99b
to
5eb55c1
Compare
Needed to upgrade to `androidx.core:core` to 1.10 where they support callStyle in `NotificationCompat`
e4cdf9b
to
6e86b52
Compare
Do I need to create a feature issue for this PR, or is the PR only OK? |
PR only is just fine - no need for administrivia like an issue just to close it, if there is a PR in flight already |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1134 +/- ##
==========================================
+ Coverage 77.08% 77.81% +0.74%
==========================================
Files 32 32
Lines 1727 1771 +44
Branches 556 592 +36
==========================================
+ Hits 1331 1378 +47
+ Misses 395 340 -55
- Partials 1 53 +52 |
PR introducing the new Android 12 CallStyle notification related to phone calls.
CallStyle can produce three different notifications:
Notes on the changes:
androidx.core:core:1.10.0
was required to have the CallSyle insideNotificationCompat
(See also this google issue about it). This update also generates the room update. Else duplicated lib errors were generatedpressActions
and create pending intent instead of behind outside of the style like the other (and be genericpressActions
props)CONTRIBUTING.md
androidx.core:core
version or even theirandroidx.core:core-ktx
to at least1.10