Skip to content
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

[#16446] Communities banner animation #16567

Merged
merged 11 commits into from
Jul 28, 2023

Conversation

ulisesmac
Copy link
Contributor

fixes #16446

Summary

This PR adds the animation for the top card banner for the communities page:

Comunity.banner.animation.webm

Review notes

The screen has the following structure:

[Communities-text]
[Discovery-card]
[Tabs]
[list of communities]

I had a problem, in figma, when the discovery card is hidden, it also affects the margin top of the [Tabs] component.
From 16:
image
to 8:
image

So the solution is not as clean as I would have liked: to avoid the animation on two components and more complex code, I just added the 8 units from the top tabs margin to the margin bottom of the discovery card.

More details in the code, don't worry, the styles are looking as in figma 👍
If you want to check the designs:
https://www.figma.com/file/h9wo4GipgZURbqqr1vShFN/Communities-for-Mobile?type=design&node-id=4996-221671&mode=design&t=NkOqJ1RAu0gpxHJ8-4

Platforms

  • Android
  • iOS

Areas that are impacted

  • Communities tab

Steps to test

  • Open Status
  • Navigate to communities tab
  • Create enough communities that doesn't fit on the list, so the scroll will be enabled
  • Scroll and the discovery banner will be animated and hidden.

Link to the animation:
https://vimeo.com/794327616

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Jul 12, 2023

Jenkins Builds

Click to see older builds (36)
Commit #️⃣ Finished (UTC) Duration Platform Result
353d8cf #1 2023-07-12 01:30:03 ~3 min tests 📄log
✔️ 353d8cf #1 2023-07-12 01:32:05 ~5 min android-e2e 🤖apk 📲
✔️ 353d8cf #1 2023-07-12 01:32:54 ~6 min android 🤖apk 📲
✔️ 353d8cf #1 2023-07-12 01:33:11 ~6 min ios 📱ipa 📲
✔️ d005d1d #2 2023-07-12 01:51:33 ~5 min android 🤖apk 📲
✔️ d005d1d #2 2023-07-12 01:51:49 ~5 min ios 📱ipa 📲
✔️ d005d1d #2 2023-07-12 01:52:01 ~5 min android-e2e 🤖apk 📲
✔️ d005d1d #2 2023-07-12 01:53:32 ~7 min tests 📄log
✔️ 2d72bd5 #3 2023-07-12 21:51:44 ~5 min android-e2e 🤖apk 📲
✔️ 2d72bd5 #3 2023-07-12 21:52:21 ~5 min ios 📱ipa 📲
✔️ 2d72bd5 #3 2023-07-12 21:54:08 ~7 min android 🤖apk 📲
✔️ 2d72bd5 #3 2023-07-12 21:54:51 ~8 min tests 📄log
✔️ e42f5bd #5 2023-07-18 03:01:49 ~8 min android-e2e 🤖apk 📲
✔️ e42f5bd #5 2023-07-18 03:01:54 ~8 min android 🤖apk 📲
✔️ e42f5bd #5 2023-07-18 03:02:21 ~8 min tests 📄log
✔️ e42f5bd #5 2023-07-18 03:10:31 ~16 min ios 📱ipa 📲
✔️ 740a4b3 #6 2023-07-18 03:46:43 ~5 min android-e2e 🤖apk 📲
✔️ 740a4b3 #6 2023-07-18 03:47:44 ~6 min ios 📱ipa 📲
✔️ 740a4b3 #6 2023-07-18 03:48:40 ~7 min android 🤖apk 📲
✔️ 740a4b3 #6 2023-07-18 03:50:04 ~8 min tests 📄log
2c60c34 #7 2023-07-18 04:26:35 ~2 min tests 📄log
✔️ 2c60c34 #7 2023-07-18 04:30:21 ~6 min ios 📱ipa 📲
✔️ 2c60c34 #7 2023-07-18 04:30:53 ~6 min android-e2e 🤖apk 📲
✔️ 2c60c34 #7 2023-07-18 04:31:00 ~6 min android 🤖apk 📲
✔️ 455bf7f #8 2023-07-18 18:16:26 ~6 min ios 📱ipa 📲
✔️ 455bf7f #8 2023-07-18 18:16:38 ~6 min android-e2e 🤖apk 📲
✔️ 455bf7f #8 2023-07-18 18:16:43 ~7 min android 🤖apk 📲
✔️ 455bf7f #8 2023-07-18 18:17:25 ~7 min tests 📄log
✔️ 585a3bc #10 2023-07-25 22:33:27 ~5 min android 🤖apk 📲
✔️ 585a3bc #10 2023-07-25 22:34:28 ~6 min ios 📱ipa 📲
✔️ 585a3bc #10 2023-07-25 22:35:38 ~7 min android-e2e 🤖apk 📲
✔️ 585a3bc #10 2023-07-25 22:36:23 ~8 min tests 📄log
✔️ 1398e09 #11 2023-07-26 22:27:07 ~8 min ios 📱ipa 📲
✔️ 1398e09 #11 2023-07-26 22:27:43 ~8 min android-e2e 🤖apk 📲
✔️ 1398e09 #11 2023-07-26 22:27:49 ~8 min android 🤖apk 📲
✔️ 1398e09 #11 2023-07-26 22:28:18 ~9 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e9f26e8 #13 2023-07-26 23:25:55 ~5 min android 🤖apk 📲
✔️ e9f26e8 #13 2023-07-26 23:26:33 ~6 min android-e2e 🤖apk 📲
✔️ e9f26e8 #13 2023-07-26 23:26:54 ~6 min ios 📱ipa 📲
✔️ e9f26e8 #13 2023-07-26 23:27:53 ~7 min tests 📄log
✔️ 530dae9 #14 2023-07-28 19:05:22 ~6 min android-e2e 🤖apk 📲
✔️ 530dae9 #14 2023-07-28 19:05:30 ~6 min android 🤖apk 📲
✔️ 530dae9 #14 2023-07-28 19:09:07 ~9 min tests 📄log
✔️ 530dae9 #14 2023-07-28 19:13:56 ~14 min ios 📱ipa 📲

@ulisesmac ulisesmac force-pushed the 16446-communities-banner-animation branch from 353d8cf to d005d1d Compare July 12, 2023 01:45
Copy link
Contributor

@ibrkhalil ibrkhalil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

src/status_im2/contexts/communities/home/style.cljs Outdated Show resolved Hide resolved
src/status_im2/contexts/communities/home/style.cljs Outdated Show resolved Hide resolved
src/status_im2/contexts/communities/home/style.cljs Outdated Show resolved Hide resolved
:elevation 1
:border-radius 16
:justify-content :space-between
:background-color (colors/theme-colors colors/white colors/neutral-90)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly a concern for your PR, but since you're touching the banner component, you could make it themeable by using with-theme and passing the theme parameter to colors/theme-colors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@flexsurfer
Copy link
Member

flexsurfer commented Jul 17, 2023

hey @ulisesmac great job, although there are two issues

  1. it feels laggy, it kinda jumps and moves with delays, not smooth
  2. if you scroll in the middle and then switch the tab, the banner appears, it shouldn't appear, or there should be a different solution from the design team

also, this needs to be done as common functionality because behavior should be the same for the chats tab as well

thanks

@ulisesmac ulisesmac force-pushed the 16446-communities-banner-animation branch from 2d72bd5 to b5dc860 Compare July 18, 2023 02:48
@ulisesmac ulisesmac force-pushed the 16446-communities-banner-animation branch 2 times, most recently from 740a4b3 to 2c60c34 Compare July 18, 2023 04:23
@ulisesmac
Copy link
Contributor Author

hey @ulisesmac great job, although there are two issues

1. it feels laggy, it kinda jumps and moves with delays, not smooth

2. if you scroll in the middle and then switch the tab, the banner appears, it shouldn't appear, or there should be a different solution from the design team

also, this needs to be done as common functionality because behavior should be the same for the chats tab as well

thanks

Hey @flexsurfer
About 1:
I changed the implementation, on lower end android devices it is still laggy, a little less but still noticeable.
The problem is I was animating the height and that is expensive, since reanimated has toi recalculate the layout; now I'm animating properties that doesn't are so expensive (transition and opacity). I created three layers and the structure of the code changed.

About 2:
I already asked, I added a temporary commit with an animation, once I know what to do I wll fix it 👍

About:

this needs to be done as common functionality because behavior should be the same for the chats tab as well

The implementation is easy to move to the chat tabs, but since its mostly about animations and layout I think is hard to create it as a common component, so I'd prefer to solve what this issue originally contemplated and add the component to the chats tab in a following PR (I can take it), wdyt?

(btw, actually, I also thought these tabs, including its banner, should have been a common component and just customize itfor each screen, maybe in a future refactor?)

@VolodLytvynenko
Copy link
Contributor

@VolodLytvynenko here's the Figma frame with the design review.

Besides that I found it laggy on my side too (iPhone 13) and on my lower end Android I wasn't even able to trigger it by scrolling the page, the animation didn't even load. Hope this help :)

@Francesca-G Thank you for your feedback. On Android it work. The issue you faced on Android is because you need to have a sufficient number of communities to enable scrolling them. I apologize for not notifying you about this

@Francesca-G
Copy link

@VolodLytvynenko here's the Figma frame with the design review.
Besides that I found it laggy on my side too (iPhone 13) and on my lower end Android I wasn't even able to trigger it by scrolling the page, the animation didn't even load. Hope this help :)

@Francesca-G Thank you for your feedback. On Android it work. The issue you faced on Android is because you need to have a sufficient number of communities to enable scrolling them. I apologize for not notifying you about this

No worries! Since I was able to drag the page down on iOS without having enough communities to scroll I was assuming it worked on Android as well

@VolodLytvynenko
Copy link
Contributor

@ulisesmac I have created as a separate follow-up #16782 of feedback details mentioned in #16567 (review)

@ulisesmac
Copy link
Contributor Author

@ulisesmac I have created as a separate follow-up #16782 of feedback details mentioned in #16567 (review)

Thanks @VolodLytvynenko

@Francesca-G I already read the comments but it's still not clear to me where exactly the fixes are needed. Is it possible to draw red lines indicating the margins/spaces/fixes required? or also overlapping the images (one on top of the other with transparency) to check the wrong alignments.
thanks!

@ulisesmac ulisesmac force-pushed the 16446-communities-banner-animation branch 2 times, most recently from c19573b to e9f26e8 Compare July 26, 2023 23:19
@Francesca-G
Copy link

@ulisesmac I have created as a separate follow-up #16782 of feedback details mentioned in #16567 (review)

Thanks @VolodLytvynenko

@Francesca-G I already read the comments but it's still not clear to me where exactly the fixes are needed. Is it possible to draw red lines indicating the margins/spaces/fixes required? or also overlapping the images (one on top of the other with transparency) to check the wrong alignments. thanks!

I updated the review (same frame)
Let me know if this is clearer :)

@ulisesmac
Copy link
Contributor Author

@VolodLytvynenko I pushed a version with smoother animations at the cost of a small delay (80ms).
Could you please check again to see if this is acceptable enough?
I could make it a little smoother, but the delay will be increased.

@status-im-auto
Copy link
Member

85% of end-end tests have passed

Total executed tests: 39
Failed tests: 6
Passed tests: 33
Not executed tests: 1
IDs of not executed tests: 702869 
IDs of failed tests: 702732,703133,702851,702783,702731,703382 

Not executed tests (1)

Click to expand
  • Rerun not executed tests
  • Failed tests (6)

    Click to expand
  • Rerun failed tests

  • Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783

    Device 2: Find `Text` by `xpath`: `//*[starts-with(@text,'test message')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@content-desc='message-status']/android.widget.TextView`
    Device 2: `Text` is `Delivered`

    critical/chats/test_1_1_public_chats.py:1379: in test_1_1_chat_is_shown_message_sent_delivered_from_offline
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Message was not delivered after resending from offline
    



    Device sessions

    2. test_1_1_chat_pin_messages, id: 702731

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851

    Device 2: Find Button by accessibility id: tab-contacts
    Device 2: Tap on found: Button

    medium/test_activity_center.py:112: in test_activity_center_contact_request_accept_swipe_mark_all_as_read
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Contact was not added to contact list after accepting contact request (as receiver)
    E    Contact was not added to contact list after accepting contact request (as sender)
    



    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133

    Device 1: Find Button by accessibility id: show-profiles
    Device 1: Tap on found: Button

    critical/test_public_chat_browsing.py:486: in test_restore_multiaccount_with_waku_backup_remove_switch
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     admin_open was not restored from waku-backup!!
    E    member_open was not restored from waku-backup!!
    E    admin_closed was not restored from waku-backup!!
    E    member_closed was not restored from waku-backup!!
    E    Incorrect contacts number restored: 0 instead of 2
    E    Contact(s) was (were) not restored from backup: Test_contact, MyCustomNickname!
    



    Device sessions

    2. test_community_mute_community_and_channel, id: 703382

    Device 1: Find MuteButton by accessibility id: mute-community
    Device 1: Click system back button

    critical/test_public_chat_browsing.py:402: in test_community_mute_community_and_channel
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Text 'Muted until 06:24 Fri 04 Aug' is not shown for a muted community channel
    



    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    Passed tests (33)

    Click to expand

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_decline, id: 702850
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_several_images_send_reply, id: 703194
    Device sessions

    2. test_community_one_image_send_reply, id: 702859
    Device sessions

    3. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    4. test_community_mark_all_messages_as_read, id: 703086
    Device sessions

    5. test_community_contact_block_unblock_offline, id: 702894
    Device sessions

    6. test_community_mentions_push_notification, id: 702786
    Device sessions

    7. test_community_message_delete, id: 702839
    Device sessions

    8. test_community_message_send_check_timestamps_sender_username, id: 702838
    Device sessions

    9. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    10. test_community_message_edit, id: 702843
    Device sessions

    11. test_community_leave, id: 702845
    Device sessions

    12. test_community_unread_messages_badge, id: 702841
    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_activity_center_mentions, id: 702957
    Device sessions

    2. test_navigation_jump_to, id: 702936
    Device sessions

    3. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    4. test_activity_center_admin_notification_accept_swipe, id: 702958
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    2. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    3. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    4. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    5. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    6. test_1_1_chat_mute_chat, id: 703496
    Device sessions

    7. test_1_1_chat_edit_message, id: 702855
    Device sessions

    8. test_1_1_chat_send_image_save_and_share, id: 703391
    Device sessions

    9. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_mute_chat, id: 703495
    Device sessions

    2. test_group_chat_send_image_save_and_share, id: 703297
    Device sessions

    3. test_group_chat_reactions, id: 703202
    Device sessions

    4. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    5. test_group_chat_offline_pn, id: 702808
    Device sessions

    @VolodLytvynenko
    Copy link
    Contributor

    Hi, @ulisesmac looks better now. Thank you for the PR. It's ready to be merged.

    Just to summarize:

    @ulisesmac ulisesmac force-pushed the 16446-communities-banner-animation branch from e9f26e8 to 530dae9 Compare July 28, 2023 18:58
    @ulisesmac ulisesmac merged commit c4b142e into develop Jul 28, 2023
    2 checks passed
    @ulisesmac ulisesmac deleted the 16446-communities-banner-animation branch July 28, 2023 19:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    Fade out Discover Communities banner when scrolling down
    9 participants