-
Notifications
You must be signed in to change notification settings - Fork 986
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
[#18453] feat: implement change accent colour screen #18517
Conversation
Jenkins BuildsClick to see older builds (54)
|
7e8fe9a
to
61fe7a2
Compare
src/status_im/contexts/chat/messenger/messages/transport/events.cljs
Outdated
Show resolved
Hide resolved
9b7b88c
to
8283f71
Compare
67% of end-end tests have passed
Failed tests (12)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Expected to fail tests (4)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (32)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
77% of end-end tests have passed
Failed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
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.
Good work @mohsen-ghafouri!
A small side note. I noticed the you added the review changes by amending (i guess) the previous commit in 8283f71 and force-pushing. It makes it a bit difficult to track what changed post-review.
@clauxx Oh my bad, i usually do. here I used amend by mistake. sorry for that. |
Hey @mohsen-ghafouri could you rebase both go and mobile PRs against latest develops please? |
8283f71
to
1d7daaa
Compare
79% of end-end tests have passed
Failed tests (9)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
22% of end-end tests have passed
Failed tests (7)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (2)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Thanks for your work @mohsen-ghafouri! @mohsen-ghafouri would you also mind creating a follow-up for Issues 2/3, please? |
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.
Here's the review
There's also a couple of issues in the transition and the animation:
-
the Accent Color screen shouldn't come from the bottom, it should come from the right (it has a back button so the transition is horizontal)
-
the color picker should be scrolled already to the selected color, without the fast horizontal scroll animation (this happens when you select one of the last colors)
RPReplay_Final1707235189.mp4
0f3b6d7
to
bc3a525
Compare
Thank you @Francesca-G, could you please check again? also I resolved the transition for edit profile and edit name screen as they also have back button too Simulator.Screen.Recording.-.iPhone.13.-.2024-02-07.at.09.17.02.mp4 |
Thank you, transition and animation issues look fixed now 👍 Design: Implementation: |
bc3a525
to
9d45810
Compare
Sorry @Francesca-G, could you please check again. |
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.
@mohsen-ghafouri looks good now 👍
9d45810
to
f931d7f
Compare
f931d7f
to
f8db861
Compare
fixes #18453
Summary
Implement change accent colour
Steps to test
Result
Simulator.Screen.Recording.-.iPhone.13.-.2024-01-23.at.19.18.06.mp4
status: ready