-
Notifications
You must be signed in to change notification settings - Fork 987
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
Conversation
Jenkins BuildsClick to see older builds (36)
|
353d8cf
to
d005d1d
Compare
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.
lgtm
:elevation 1 | ||
:border-radius 16 | ||
:justify-content :space-between | ||
:background-color (colors/theme-colors colors/white colors/neutral-90) |
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.
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
.
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.
Done 👍
d005d1d
to
2d72bd5
Compare
hey @ulisesmac great job, although there are two issues
also, this needs to be done as common functionality because behavior should be the same for the chats tab as well thanks |
2d72bd5
to
b5dc860
Compare
740a4b3
to
2c60c34
Compare
Hey @flexsurfer About 2: About:
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?) |
@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 |
@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. |
c19573b
to
e9f26e8
Compare
I updated the review (same frame) |
@VolodLytvynenko I pushed a version with smoother animations at the cost of a small delay (80ms). |
85% of end-end tests have passed
Not executed tests (1)Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (33)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Hi, @ulisesmac looks better now. Thank you for the PR. It's ready to be merged. Just to summarize:
|
e9f26e8
to
530dae9
Compare
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:
I had a problem, in figma, when the discovery card is hidden, it also affects the margin top of the
[Tabs]
component.From 16:
to 8:
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
Areas that are impacted
Steps to test
Link to the animation:
https://vimeo.com/794327616
status: ready