[#18595] Implement collectible header with animations #20024
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
ulisesmac
changed the title
ulisesmac
changed the title
|
Replying to what @OmarBasem said in the previous PR:
I'm not sure about QA since it is just a visual change, and we implemented this during the offsite, we had Mario's feedback and approval about the animations, behavior and timings. cc: @J-Son89 wdyt? should we ask for QA and design review? |
|
We had some issues compiling because of the JS worklet location, but AFAIK it was solved, now @mohsen-ghafouri found a compilation issue 🤔 any idea of what is happening? |
Jenkins BuildsClick to see older builds (24)
|
85% of end-end tests have passedFailed tests (6)Click to expandClass TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
Since it's a large pr and we had design feedback already maybe it's best to get merged and we can adjust minor details as part of a larger ensign review? |
If the PR is just adjusting some visuals, and had no previous problems with animation performance then fine. Otherwise, better go through QA imo. |
Not sure whats going on. I didn't face any issues while development. I will check and update you. |
ajayesivan
force-pushed
the
18595-collectible-header
branch
2 times, most recently
from
9bb696a
to
d23f1c8
Compare
|
Running this PR was throwing the flowing error on launch, This issue was due to the requires in the even though cc: @mohsen-ghafouri |
ajayesivan
force-pushed
the
18595-collectible-header
branch
from
2cb518e
to
b36d655
Compare
| :title collectible-name | ||
| :description collection-name | ||
| :theme theme}] | ||
| [reanimated/scroll-view |
There was a problem hiding this comment.
hi, why this is reanimated view instead of simple view?
There was a problem hiding this comment.
Hi @Parveshdhull !
If you are referring to the reanimated/scroll-view, the reason is we are capturing its on-scroll callback to animate a shared-value, since we wanted it to properly report the data, I thought it was better to use the reanimated one.
There was a problem hiding this comment.
hi Thank you for giving the context.
Most probably we will not get any benefit using reanimated here, unless we are using animated-scroll-handler something like
There was a problem hiding this comment.
Thanks for the suggestion, I'll try removing it and using a normal scroll view to check if it behaves the same 👍
| ); | ||
|
|
||
| export function useLayerOpacity(sharedValue, from, to) { | ||
| return useAnimatedStyle(() => ({ |
There was a problem hiding this comment.
This approach has few drawbacks.
- I am not sure if situation is changed, but changes in js code were not visible until app restarted
- Also for consistency we should stick to a single approach. If we use this approach everywhere we will have three files, view, simple style, and animated style, for all animations and we have several animations.
There was a problem hiding this comment.
I am not sure if situation is changed, but changes in js code were not visible until app restarted
It hasn't changed 😢 and I know, it's a shame that we need to restart the app, I'm not sure if the REPL works to reload the JS requires.
Also for consistency we should stick to a single approach. If we use this approach everywhere we will have three files, view, simple style, and animated style, for all animations and we have several animations.
Yes, I'm not a big fan of this approach too, that's why I first tried to solve it in a different way:
although, I don't exactly like it, it solves the following existing problems:
- We are always transforming CLJS styles to JS (which we know is expensive)
apply-animations-to-styleis using a hook inside, but devs don't know it, and we can find A LOT of usages of this hook as a regular function, so it's not declared at the beginning, for example.- Our
interpolate-valueis not just a call to reanimated's interpolate (which is just a regular function), it hides a hook (useDerivedValue) so it has the same problems as mentioned in2Additionally, we are making the code more complex, since instead of interpolate directly inside auseAnimatedStyle, we generate derived values with ourinterpolate. transform CLJS -> JS values, then pass them toapply-animations-to-styleand transform them again.
To show the complexity added because of our wrappers, take a look at this piece of code (taken from the PR I mentioned, it is/was used to animate the avatar size).
With our wrappers
(defn f-avatar
[...]
;; Interpolate, which is creating a derived value, it's a hook but looks as a regular function
(let [image-scale-animation (reanimated/interpolate scroll-y
scroll-animation-input-range
[1 0.4]
header-extrapolation-option)
;; 2nd interpolate usage, CLJS->JS transform, and useDerivedValue hook implied
image-top-margin-animation (reanimated/interpolate scroll-y
scroll-animation-input-range
[0 20]
header-extrapolation-option)
;; 3rd interpolate usage, CLJS->JS transform, and useDerivedValue hook implied
image-side-margin-animation (reanimated/interpolate scroll-y
scroll-animation-input-range
[0 -20]
header-extrapolation-option)
theme (quo.theme/get-theme)]
[reanimated/view
;; `style/avatar-container` Looks just as a regular function, but it is/was using
;; `apply-animations-to-style`, which is terrible, because that's a hook, and we
;; aren't declaring it at the top, also it looks as a style fn, but it does more.
{:style (style/avatar-container theme
image-scale-animation
image-top-margin-animation
image-side-margin-animation)}
...]))Without our wrappers
(defn f-avatar
[...]
(let [theme (quo.theme/get-theme)
avatar-animation (worklets/use-avatar-animation scroll-y input-range #js[1 0.4] #js[0 20] #[0 -20])]
[reanimated/view {:style [(style/avatar-container theme) ;; <- actually only styles
avatar-animation]}
...]))export function useAvatarAnimation(scrollY, inputRange, range1, range2, range3) {
return useAnimatedStyles(() => (
{
transform: [
// Simplifyed calcs, maybe there were some other operations, but we can perform the interpolate here
// No need of extra sharedValues created with `useDerivedValue`
{scale: interpolate(scrollY.value, inputRange.value, range1)},
{translateX: interpolate(scrollY.value, inputRange.value, range2)},
{translateX: interpolate(scrollY.value, inputRange.value, range3)}
]
})
)
}The code is smaller and more straightforward, additionally, we are following the recommended practices for hooks.
@Parveshdhull WDYT?
There was a problem hiding this comment.
Thank you very much for sharing detailed explanation and example.
From your example, it seems for the use case of interpolate it looks simpler to just define whole style in js.
But for other use case, I think we should use wrapper for now.
Otherwise we will lose the ability of hot reloading style related changes. And debugging and developing will be harder.
apply-animations-to-style is using a hook inside, but devs don't know it, and we can find A LOT of usages of this hook as a regular function, so it's not declared at the beginning,
I think this problem can be easily solved. We just need to communicate to devs and add to guideline.
There was a problem hiding this comment.
There's already a standard in react to prefix hooks with 'use-'
There was a problem hiding this comment.
We probably should rename this wrapper then. thank you @J-Son89
There was a problem hiding this comment.
@Parveshdhull we could:
- Rename the wrappers adding
use-...to the ones that involve a hook and keep using them. - Be aware that sometimes writing a worklet directly in JS may be cleaner and simpler in terms of code, so we can take the JS approach, for example, for complex animations (like the ones having multiple interpolations).
wdyt?
I've tried to create worklets in CLJS in different ways and I've got partially working results. I'm just trying to find a way to improve our animated code and API 🙃
There was a problem hiding this comment.
sounds good to me. Thank you for looking into this.
Merged
The animation perf is good 👍 so yeah, let's skip QA for this one |
ulisesmac
force-pushed
the
18595-collectible-header
branch
from
cdb071f
to
89eaabd
Compare
|
@status-im/mobile-qa We'll skip QA for this one, but could you please make sure the e2e test are passing? thanks! |
71% of end-end tests have passedFailed tests (13)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
|
mohsen-ghafouri
approved these changes
May 16, 2024
69% of end-end tests have passedFailed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (9)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
Hi @ulisesmac, thanks for the PR! E2E failures are not related |
ajayesivan
force-pushed
the
18595-collectible-header
branch
from
89eaabd
to
39f3552
Compare
Co-authored-by: Ajay Sivan <ajayesivan@gmail.com>
ajayesivan
force-pushed
the
18595-collectible-header
branch
from
39f3552
to
af29ae3
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 #18595
This PR is the continuation of:
It was merged but it lacks of e2e tests and QA. Also @mohsen-ghafouri found some issues in the compilation.
The PR had dev approvals, in this PR we want to identify and solve the compilation issues
status: WIP