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
Collectible header #19783
Collectible header #19783
Conversation
Jenkins BuildsClick to see older builds (59)
|
e10af72
to
16b10e5
Compare
65a46c2
to
4f842fc
Compare
67509b4
to
35b2486
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.
Nice work, Left some nits/concerns.
;; We need to delay the measurement because the navigation has an | ||
;; animation | ||
(js/setTimeout #(when @title-ref | ||
(.measureInWindow @title-ref set-title-bottom)) | ||
300))}]] |
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.
Can this delay be done using the withDelay
function of reanimated in the .js file?
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.
but this is not an animation. The timeout is here to make sure measureInWindow
is called later in time
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.
Amazing job, guys! Was a pleasure to contribute to this. Just added a small suggestion to JS code 🚀
83e5b19
to
4c2e9e4
Compare
@FFFra I tried your code style suggestions but they are being changed by |
Co-authored-by: Ajay Sivan <[email protected]>
8f79a3f
to
3a27fda
Compare
@clauxx @FFFra @ibrkhalil I already solved the problems, could you please check it again? cc: @ajayesivan |
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.
✅
@ulisesmac don't we need QA or design review for this PR? |
This reverts commit 5a6a0f7.
[reanimated/scroll-view | ||
{:style (style/scroll-view top) |
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.
hi, why this is reanimated view instead of simple view?
return useAnimatedStyle(() => ({ | ||
flex: 1, | ||
justifyContent: 'flex-end', | ||
backgroundColor: interpolateColor(sharedValue.value, [0, 60], [from, to], 'RGB'), |
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.
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.
fixes #18595
Summary
Improve collectible header animation.
Platforms
Areas that maybe impacted
Steps to test
Known issues
(We will handle the above issues separately)
Screenshots
Android
Light Mode:
Android.-.Light.mov
Dark Mode:
Android.-.Dark.mov
iOS
Light Mode:
iOS.-.Light.mp4
Dark Mode:
iOS.-.Dark.mp4
status: ready