-
Notifications
You must be signed in to change notification settings - Fork 984
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
Set Activity Center as the new default #14492
Conversation
Jenkins BuildsClick to see older builds (8)
|
0% of end-end tests have passed
Not executed tests (3)Failed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
|
0% of end-end tests have passed
Failed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
|
hi @ilmotta please, help me with clarification of the notification receiving flow in AC. |
Hey @VladimrLitvinenko, thanks for testing the PR. Let me try to explain some details that might not be obvious. The new Activity Center is not (necessarily) going to display the same notifications displayed by the current Notification Center. What's currently implemented in the new AC is what I've shared in the PR description:
Other kinds of notifications will be supported in the next coming weeks/months. I think it's important to reinforce that the backend still generates/stores the same notifications, it's just that the new AC frontend has not yet implemented a way to display them. So the new AC has the same capabilities as the old Notification Center, but we need more time to achieve feature parity. I've just created issue #14501, as it's related to this problem we're discussing now. As I mentioned in #14501, there will be different business rules to how/when 1:1 notifications are generated, but because I haven't been working on this specific type of notification I can't answer precisely the new requirements. This is left for the person that eventually pick the issue. So, to answer your question:
I think for now we should keep the button. But perhaps we should disable the failing tests relying on a 1:1 notification being generated. Do you think we can skip the failing tests? I know it's not ideal 🤷♂️ |
Hi @ilmotta should these UI notifs be in the scope of the current PR ISSUE 1: The notif in Activity is not shown as per visual if the light mode is used
Actual result:
Expected result:
ENV: Any android device I am not sure that issues #2 and #3 are in the scope of this PR. Let me know if they should be created separately ISSUE 3. The accepted and declined contact are not shown in the Activity Center
Actual result: Expected result: |
Currently, I am blocked by #14362 to test this PR fully, because the mentions are not ready yet |
Thanks for the detailed reports @VladimrLitvinenko. Let's see:
I'm going to fix this real quick in a future PR. Good catch.
This is the expected behavior in Figma as well. In your screenshot we can see the text would not fit in a single line without overflowing the surrounding padding, so it has to be displayed in 2 rows.
This is on purpose, because the Design team has not finished the mockups for the empty state, so meanwhile it was decided to not waste time implementing that temporary empty state (the text will change, styles, etc). This is being tracked on issue Display empty tab state when there are no notifications
I tried to reproduce this issue, but it works fine for me. So could you give more detailed instructions about the problem? Which tab you had selected? Was the all/unread filter enabled? If it's not asking too much, but a video would help me reproduce :) |
Sorry, I didn't specify it should be done on Android. The record is done for android studio, but for my real device: Huawei p20 light; Android version: 9 it is reproducible as well. Also, the error pop up appears when I try to navigate to any tab: "all", "admin", "mention" etc. that also visible on the record |
@VladimrLitvinenko
I exclusively use GNU/Linux and Android Studio :)
I've never seen this error. I'll try to reproduce using other physical devices I have here, because it never happened to me in Android Studio. About the disappearing notification, this is expected according to the current design. It vanished because once you interact with a notification with call to action, the notification is marked as read, but because you had the filter enabled to only display unread notifications, it disappeared. If you accept/reject the contact request with the filter disabled (i.e. show both read/unread), then you'll still see the notification. As far as I remember, designers chose a behavior similar to WhatsApp or some other known product. I think some sort of animation could be helpful to users, to somehow hint what happens when a notification is marked as read. Some animations will be implemented before the MVP is released @VladimrLitvinenko. I'm taking note of your feedback for future reference. |
Visually looks good in dark mode, not in light mode. Bug wise, still having problems |
Currently, as a user, I am not able to mention other users in chats using @. It is described in #14362 in ISSUE 1. |
Oh. Now I got it. Thank you! |
@VladimrLitvinenko, @Du64: I'll try give a round of review for light mode and create an issue to fix these type of errors later. |
@Du64, which "same error" are you referring to? |
It's a bit more complicated @VladimrLitvinenko. As I mentioned previously, I've created the issue #14501, which is related to how mentions will be displayed in the new Activity Center. The business rules are different, and some of them still need the blessing/review from designers. We're not quite ready to play that issue. I believe this will affect your testing processes meanwhile. |
@ilmotta this is the bug I was referring to, sent the wrong screenshot before |
Sure @churik, I just committed to add an accessibility label named |
👍 I think your suggestion is the most sensible @churik. According to the new rules (yet to be set in stone), it could well be the case that mention notifications won't be generated in a way that's very testable (e.g. mentions could be aggregated for a certain amount of time to avoid spamming the user with too many in a short period of time). |
100% of end-end tests have passed
Passed tests (8)Click to expandClass TestDeeplinkOneDeviceNewUI:
Class TestGroupChatMediumMultipleDeviceNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Tests are fixed in #14506 |
That's great news @churik! Thank you. This PR will receive a fix when I find out why a RN function is undefined after the app is released (error popup #14492 (comment)) I'll request your reviews again once I find the fix. |
f077ab6
to
4a3f3aa
Compare
@ilmotta thank you for your work. Soon it'll be tested |
100% of end-end tests have passed
Not executed tests (2)Passed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeeplinkOneDeviceNewUI:
Class TestGroupChatMediumMultipleDeviceNewUI:
|
@ilmotta testing for PR is passed and ready to be merged. Thank you! |
|
4a3f3aa
to
9c31dd1
Compare
Partially implements #14490
Summary
This tiny little PR simply enables the new Activity Center (AC) by default. A follow up PR will then remove the old Notification Center code and the flag. The goal is for the AC to be usable in the next release, so we can dogfood the redesigned mobile app.
Types of notifications supported by the new AC as of 2022-12-06
Why?
This PR is needed because I want to be sure the new AC won't disrupt the QA team in the first place. Also because I want the next PR (not yet created) that will remove the old Notification Center to be merged as fast as possible, in order to avoid code conflicts. Hence why enabling the flag is coming before code deletion.
Platforms
Steps to test
The most important case to test IMO is the Contact Request flow, because it's a requirement to chat with other users. To test the Activity Center, simply click on the bell icon in the home screen.
There are many issues already created mapping the work left to be done for the AC. You can check them out here https://github.com/status-im/status-mobile/labels/Activity%20center.
Figma flows: https://www.figma.com/file/eDfxTa9IoaCMUy5cLTp0ys/Shell-for-Mobile?node-id=451%3A18681&t=gIS1Vt4Ucc2gSrlr-0
Note for QA review
status: ready