-
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
feat: finish enable notifications #18084
Conversation
Jenkins BuildsClick to see older builds (20)
|
0176537
to
b56e8b2
Compare
[parallax/video | ||
{:layers (:notifications resources/parallax-video) | ||
:stretch stretch}])) | ||
|
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.
extra line
@@ -207,7 +207,7 @@ | |||
:animations (merge transitions/new-to-status-modal-animations | |||
transitions/push-animations-for-transparent-background) | |||
:modalPresentationStyle :overCurrentContext} | |||
:component enable-notifications/enable-notifications} | |||
:component enable-notifications/view} |
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 tested this and looks good 👌 - admittedly the app did crash on the onboarding process but on the "welcome" page and I couldn't reproduce this afterwards, only happened once out of a few trys |
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 ✨
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.
Nice work 🎉
b56e8b2
to
730f57f
Compare
730f57f
to
06da9ff
Compare
82% of end-end tests have passed
Failed tests (2)Click to expandClass TestActivityMultipleDevicePR:
Expected to fail tests (7)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (40)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
|
06da9ff
to
4316259
Compare
@BalogunofAfrica thank for the PR and sorry for delay with testing. No issues from my side. Ready for design review @Francesca-G |
Thanks @pavloburykh @Francesca-G has already reviewed |
Great! Then we are ready for merge. |
e48aff4
to
8b5883a
Compare
fixes #17506
Summary
This PR implements the enable notifications screen as seen in the design feedback here: https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=6117-67315&mode=design&t=DAsT60Sje05FrhhM-0#569642792
Platforms
status: ready