-
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
Add conversation styling and shortcuts for Android 🤖 #47626
Add conversation styling and shortcuts for Android 🤖 #47626
Conversation
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
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.
Looks good other than one more change
|
||
builder.setStyle(messagingStyle); | ||
// Conversational styling should be applied to groups chats, rooms, and any 1:1 chats with more than one notification (ensuring the large profile image is always shown) | ||
if (!conversationName.isEmpty()) { |
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.
if (!conversationName.isEmpty()) { | |
if (!roomName.isEmpty()) { |
Can you rename this variable to make it a little more clear?
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.
Changed 🫡
This comment has been minimized.
This comment has been minimized.
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 feels pretty good 🚀
screen-20240819-212547.2.mp4
@staszekscp - I'm seeing an empty option in the menu when there is a notification: |
Is this expected? If we have two notifications only one of them is displayed in the long press menu: screen-20240819-215535.mp4 |
I think that's how Android handles it - on other apps it only shows the newest notification, and it looks very similar to the video |
@staszekscp - Is this expected also: #47626 (comment)? |
@rayane-djouah I haven't found any way to change that. I also quickly browsed through the Google documentation, and couldn't see anything that could change that behaviour - nevertheless it only appears when there are less than 3 shortcuts. I'll investigate that further However I've also noticed a small bug I'll have to fix - the shortcuts don't get dismissed after signing out. I'll try to address that this week! (I also have some duties around the HybridApp, that's why I cannot jump straight to it) |
Just a quick update: I've had some time to give it a go. My guts tell me it's possible, but I was still not able to change the behaviour of the shortcut 😄 I also investigated the shortcuts removal upon sign out, however it would be a bit more complicated. Ideally I would write a native class that would handle the shortcuts, and expose the There are some RN libraries out there, but it seems they don't handle our specific use-case of Conversation Shortcuts. Therefore I don't know if they would work correctly. We can always write a tailor-made Native Module that could also handle our navigation quirks if we decide to add other shortcuts (similarly to OldDot) I would love to hear your feedback! I'll try to find some time next week to move on with it! |
This issue happens on every flavor of Android (Pixel, Samsung, etc) or just some of them?
After signing out, do the shortcuts not work at all? Or does the app let you sign in and then redirect to the correct chat? |
Hi everyone!
I tested on 3 devices, and it seems like it might be the default Android behaviour in that scenario (if there is a Notification targeting one of shortcuts, it doesn't duplicate the data). Although if there is more than 2 shortcuts, the issue doesn't exist (the problematic empty shortcut doesn't show up). I played with shortcut and notification builders, but I couldn't find a way to get rid of this problem
The shortcuts are available, but if the user is logged out the app opens on the SignIn page. If the user is signed in to another account, the |
Oh okay good catch. Then we should be sure to reset these shortcuts whenever we log out/in. |
Sorry for answering late, we've had some work after upgrading React Native in Hybrid App 😄
So I think that the cleanest solution would be to create a simple |
I can't think of any other option so that makes sense to me too 👍 |
Thanks for answering @arosiclair! In this case, let's go with the |
Hi, I'm Michał from SWM and I will continue finishing this PR 😄 |
Hello, I'm having trouble with testing message notifications when app is in the background. Do we have any problems with notifications system right now? I checked the production Android app and I don't get any alert/pop up when other users text me. The same happens on the latest main and on this PR's branch. Any updates on the backend side? |
Yes we started sending NewDot push notifications only to users that are enrolled in the new experience ( const nvp = NVP.get('tryNewDot');
nvp.classicRedirect.dismissed = false;
NVP.set('tryNewDot', nvp); Alternatively, creating a new user in NewDot should automatically enroll them. |
Heads up push notifications for the new experience are indeed bugged. We're working on a fix. Hopefully we can get it out in the next day or two. |
@arosiclair Could you please trigger an adhoc build? |
Build is running here |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Weird
https://github.com/Expensify/App/actions/runs/10704914058/job/29679050710 |
@rayane-djouah can you build and test locally? |
@arosiclair Hmm I see that there were some problems with the builds, maybe they was caused by not having the latest changes. I just merged the latest main so how about trying building it again? |
We should just continue testing locally, ad-hoc builds are very slow and inefficient especially if we don't really need them. |
@rayane-djouah Have you been able to test it with notifications enabled? |
Bug:
screen-20240911-220132.mp4cc @Skalakid |
Bug: Conversation shortcuts don't work and sometimes take to a not found page: screen-20240911-221419.mp4screen-20240911-221643.mp4cc @Skalakid |
@rayane-djouah, I've added a fix for it. After these changes, I wasn't able to reproduce the issues that you mentioned :D |
Reviewer Checklist
Screenshots/VideosAndroid: Nativescreen-20240913-004530.mp4screen-20240913-005120.mp4screen-20240913-005204.mp4Android: mWeb ChromeN/A iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-13.at.01.12.32.mp4Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-13.at.01.11.05.mp4iOS: mWeb SafariN/A MacOS: Chrome / SafariN/A MacOS: DesktopN/A |
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.
LGTM and tests well
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.
Looks great overall just one change needed
src/libs/actions/Session/index.ts
Outdated
@@ -205,6 +206,7 @@ function hasAuthToken(): boolean { | |||
function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSession?: boolean, killHybridApp = true) { | |||
Log.info('Redirecting to Sign In because signOut() was called'); | |||
hideContextMenu(false); | |||
ShortcutManager.removeAllDynamicShortcuts(); |
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.
I think it would probably be better to put this in PushNotification.deregister. Can we do a quick test to make sure it still works correctly from there?
@arosiclair I've added your suggestion, and everything seems to work well 😄 Screen.Recording.2024-09-16.at.13.30.57.mov |
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.
Great work thank you!
✋ 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/arosiclair in version: 9.0.36-0 🚀
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.36-2 🚀
|
FYI @Julesssss we deployed conversation notifications for Android (NewDot not HybridApp) with this ^. I'm not on Android anymore so I can't test it normally so lemme know if this is buggy in any way. |
cc: @arosiclair
Details
The PR implements conversation notifications for Android alongside with conversation shortcuts. From now on it is possible to go to a conversation from android shortcuts, and manage them in system options.
Fixed Issues
$ #37604
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
w_4b330eb7fc4209cb67f94012d6eda552b59ace52-2024-08-19.09_47_18.477.mp4
Android: mWeb Chrome
N/AiOS: Native
N/AiOS: mWeb Safari
N/AMacOS: Chrome / Safari
N/AMacOS: Desktop
N/A