-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Improve Android notification threading, categorisation, and profile images #19613
Conversation
# Conflicts: # android/app/src/main/AndroidManifest.xml
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:tools="http://schemas.android.com/tools" | ||
package="com.expensify.chat"> | ||
xmlns:tools="http://schemas.android.com/tools" | ||
package="com.expensify.chat"> | ||
|
||
<uses-permission android:name="android.permission.INTERNET" /> | ||
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/> | ||
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> | ||
<uses-permission android:name="android.permission.CAMERA" /> | ||
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/> | ||
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/> | ||
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> | ||
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" /> | ||
|
||
<!-- android:hardwareAccelerated is essential for Android performance: https://developer.android.com/topic/performance/hardware-accel --> | ||
<application | ||
android:supportsRtl="false" | ||
android:hardwareAccelerated="true" | ||
android:name=".MainApplication" | ||
android:label="@string/app_name" | ||
android:icon="@mipmap/ic_launcher" | ||
android:roundIcon="@mipmap/ic_launcher_round" | ||
android:allowBackup="false" | ||
android:resizeableActivity="false" | ||
android:theme="@style/AppTheme" | ||
tools:replace="android:supportsRtl"> | ||
|
||
<activity | ||
android:name=".MainActivity" |
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.
Most of the manifest changes are fixing bad formatting. I will comment to point out the actual changes.
<meta-data | ||
android:name="com.google.firebase.messaging.default_notification_icon" | ||
android:resource="@drawable/ic_launcher" /> |
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.
This is a new addition. It sets a default notification icon
<meta-data | ||
android:name="com.google.firebase.messaging.default_notification_color" | ||
android:resource="@color/bootsplash_background" /> |
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.
This is another addition, it sets the default notification background color.
🎯 @eVoloshchak, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #21554. |
The automation is clearly broken...I'm going to create a new issue and then use that to manually assign a randomly assigned internal engineer. |
@Julesssss I think we should post this: 🎀 👀 🎀 C+ reviewed Edit: it didn’t trigger the automation as well 😕 |
I'll try pullerbear again. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry everyone who is being tagged, I'm trying to figure out how the new automation works. |
Okay, @bondydaa won the prize of being the internal reviewer for this PR. Let me know if you need any help testing, but it should be pretty straightforward if you use the Android build here. You can test using the staging/prod environment 👍 |
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.
nothing seems glaring wrong but I also am def not an Android expert here either.
Maybe @cristipaval might want to double check this too, I think you have some more native experience?
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.34-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.34-1 🚀
|
Details
Introduces conversation styel notifications. iOS and Android notifications now show a bigger avatar of the message sender. On Android, we also include much better notification threading, with mini conversations displayed within the notification along with mini avatars for each user.
Improvements to Android notification threads:
Minor improvements
DM me if you want a pre-compiled release APK to test on an Android device or emulator!
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/260137
Tests
1) Test Direct message notifications
Chat With:
Hari Seldon
[lots of emojis]
2) Test Group chat noifications
jules, Jules, Hari, Salvor - now
Hari Seldon: Lets all go out for lunch today?
3) Test room notificatons
Immediately
New Expensify - #announce - now
Hari Seldon
Welcome new employees!
[x] Verify that no errors appear in the JS console
Offline tests
Not applicable, as you must be online in order to receive notifications!
QA Steps
Run the same tests as above!
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Only Android was modified!
Many screenshots are shown against the test cases.