fix: Community logo and title animation #19651
Merged
+90
−82
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 (26)
|
ajayesivan
force-pushed
the
19635-community-logo-and-title-animation-fix
branch
2 times, most recently
from
c345688
to
c6e2e9c
Compare
ajayesivan
requested review from
vkjr,
smohamedjavid,
ulisesmac,
Parveshdhull and
BalogunofAfrica
ajayesivan
force-pushed
the
19635-community-logo-and-title-animation-fix
branch
from
c6e2e9c
to
4b1e2bb
Compare
ajayesivan
force-pushed
the
19635-community-logo-and-title-animation-fix
branch
from
4b1e2bb
to
a9210d4
Compare
Parveshdhull
reviewed
May 8, 2024
| {:opacity center-opacity} | ||
| nil) | ||
| (style/center-content-container | ||
| (if (= type :title) (= text-align :center) false))) |
There was a problem hiding this comment.
center-content-container (and (= type :title) (= text-align :center))?
Parveshdhull
approved these changes
May 8, 2024
seanstrom
reviewed
May 9, 2024
Comment on lines
261
to
266
| (let [center-content-container-style (reanimated/apply-animations-to-style | ||
| (if center-opacity | ||
| {:opacity center-opacity} | ||
| nil) | ||
| (style/center-content-container | ||
| (if (= type :title) (= text-align :center) false))) |
There was a problem hiding this comment.
Hey @ajayesivan 👋 Thanks for the PR 🙏
I have some questions around this refactor, and I want to make sure I understand the code:
- I was noticing that some components like
wallet-networks-centeranddropdown-centerwere always passingcentered?astrue. Should they continue to do that? - I was also noticing that most of the other components like
channel-centeralways passedcentered?asfalse. And thattitle-centerwas the only one that checked for(= text-align :center). Should they all now only be centered based on text-align?
I suppose it makes sense that we have a standard way of determining when something should be centered, though I wonder if the code had some special assumptions by always passing false or true.
There was a problem hiding this comment.
Hi @seanstrom, yes, it is a bit confusing and maybe not the ideal solution for this. But first of all, I'm not trying to fix that in this PR. My goal in this PR is to fix the animation.
In this component, only the type title has two alignments: center and left. So the text-align property is only considered when the type is title.
Design: https://www.figma.com/design/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile?node-id=159-6028&m=dev
Hi @ajayesivan Did you find the root reason for this bug to happen? From what can I see and since I'm not familiar with this component: Another cause might be that it takes some time to render or fetch the content of the community, and the flat list is shorter at the beginning so we display the header. |
|
Hi @ulisesmac, in this component, we were using the |
ajayesivan
force-pushed
the
19635-community-logo-and-title-animation-fix
branch
2 times, most recently
from
2fd04c5
to
d871bef
Compare
ulisesmac
approved these changes
May 15, 2024
85% of end-end tests have passedFailed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ajayesivan
force-pushed
the
19635-community-logo-and-title-animation-fix
branch
from
d871bef
to
2cc3d20
Compare
|
Hi @ajayesivan ! Thanks for your fix. E2E are known and not related. |
ajayesivan
force-pushed
the
19635-community-logo-and-title-animation-fix
branch
from
2cc3d20
to
edb5b40
Compare
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 #19635
Areas that maybe impacted
Steps to test
Before and after screenshots comparison
status: ready