Implement animations for discover communities screen #17586
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (28)
|
J-Son89
reviewed
Oct 9, 2023
J-Son89
reviewed
Oct 9, 2023
J-Son89
reviewed
Oct 9, 2023
| @@ -52,18 +52,21 @@ | |||
| (def ^:const communities-discover 9) | |||
|
|
|||
| ;; Floating Screens | |||
There was a problem hiding this comment.
not for this pr, but it feels to me like this sort of information should be defined in one place, e.g navigations.screens and we have a mechanism to pass this down to the relevant places of the app.
right now it feels like we have two navigation systems, the main one and the jump to navigation.
Is this the approach we want to take or would it suit better to try to keep on navigation system?
cc @smohamedjavid, @briansztamfater
There was a problem hiding this comment.
hi @J-Son89,
navigation.screens is parent ns which holds all the screens. If we use/import this namespace in another file like here it will create cyclic dependency.
Is this the approach we want to take or would it suit better to try to keep on navigation system?
By navigation system you mean RNN?
RNN is not customizable enough to allow us create UI/animations as per design teams requirment.
There was a problem hiding this comment.
hmm, it is possible to add custom transition animations with the RNN library, as @briansztamfater has done this on other pages. see the onboarding transitions. Although I am not sure whether that is possible here and not necessarily talking about that.
By navigation system I am referring to everything related to navigation in our codebase. Not specifically about the tooling used, RNN etc.
In particular I have some questions about the code structure we are taking here.
We have navigations/screens which is the core place to store information about screens and then we have this shell information about floating screens (among other info)
imo it is worth considering that we have all navigations pieces in one location, and can create some mechanism to define this information in one place and retrieve it in sub-mechanisms of the application code, such as the shell navigation.
b36d4dc to
6dedd9e
Compare
siddarthkay
reviewed
Oct 12, 2023
72% of end-end tests have passedFailed tests (12)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (31)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
|
5138bdb to
f6d8ac3
Compare
70% of end-end tests have passedFailed tests (13)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (30)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@Parveshdhull thank you for the PR! Could you please clarify what has changed in closing animation of Discover community screen? I have compared PR and nightly builds and not able to see the difference. Below is video comparing PR and nightly builds telegram-cloud-document-2-5289565096211658295.mp4 |
ISSUE 1 User is still being navigated to Community Home screen when closing community if it has been opened from ShellSteps:
Actual result: user is navigated to Communities home screen instead of Discover community home screen telegram-cloud-document-2-5289565096211658312.mp4Expected result: user is navigated to Discover community home screen |
ISSUE 2 Jump-to button overlays community card on Discover community screen (Android)Reproducing on Samsung Galaxy A52, Android 12 Steps:
Actual result: Jump-to button overlays community card . At the same time community list is not scrollable because there are only few community cards in the list.
telegram-cloud-document-2-5289565096211658344.mp4Expected result: not sure what is expected result. Maybe user should be able to scroll the list even if there are only few cards. Otherwise jump to will overlay the card and there is nothing user can do with that. |
|
@Parveshdhull fixed the failed group, thank you for taking into account automation! |
|
hi Pavlo, Issue 2 should be fixed now. Issue 1 - I confirmed with the design team and yes communities discover screen also should be opened in the background when opening the community from the jump-to card. I will log this with the animation issue and will implement it separately.
Please check the discover communities opening/closing animations in the issue #17621. Not sure if the video is old and animations are already fixed in the develop. If this is the case please ignore the issue #16115, and only test #17621 as PR implements this animation also for shell navigation. |
e7049c7 to
57f2349
Compare
84% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (36)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@Parveshdhull Thank you for your work! PR is ready for merge. |
@Parveshdhull UPDATE: ready for design review. cc @Francesca-G |
Francesca-G
reviewed
Oct 17, 2023
@Francesca-G thank you for your review! This bug is not in scope of current PR. This particular issue has been already reported by designers in 1.25 review https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=6117-85237&mode=design&t=M4cC4712cKbrlp0Q-0#567182752 and is logged #17557 |
57f2349 to
975e75f
Compare
975e75f to
6acae42
Compare
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #16115
fixes #17621
Summary
PR implements animations for the discover communities screen and allows the opening of the community from this screen without needing it to be closed.
Note:- Currently, we are not animating closing of discover communities screen on press of jump-to button, it will be implemented separately.
Testing
Please also test #16438 (comment)
status: ready