Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
M-i-k-e-l
added
the
Important for Next Release
✅ PR Description Validation PassedAll required sections are properly filled out:
Your PR is good for review! 🚀 This validation ensures all sections from the PR template are properly filled. |
lidord-wix
left a comment
lidord-wix
left a comment
There was a problem hiding this comment.
Looks good, I added some comments, most of them are small or not so important but worth fixing :)
| <View flex /> | ||
| </View> |
There was a problem hiding this comment.
this is minor, but you can remove these two Views and pass containerStyle={styles.rowContainer} to the SegmentedControl instead
There was a problem hiding this comment.
I copy-pasted the other parts, technically it should be using ExampleScreenPresenter all over this
| isAndroidEdgeToEdge = !!androidVersion && androidVersion >= 35 ? true : undefined | ||
| } = props; | ||
|
|
||
| const animationType = animationDurationProp === 0 ? 'none' : animationTypeProp; |
There was a problem hiding this comment.
animationType (a plain JS value) is used inside useAnimatedStyle, but Reanimated worklet closures capture the value at creation time. Since animationType is derived from props and can change, the worklet will reference a stale value.
If the user switches from 'slide' to 'fade' at runtime, the worklet may still branch on the old value. Consider using a useSharedValue for animationType and updating it in a useEffect, so the worklet always reads the current value. Same concern applies to keyboardBehavior.
IMO it's not a common case but worth fixing it
There was a problem hiding this comment.
What is the possible bug?
The animationType is changed in the screen and it works as expected.
There was a problem hiding this comment.
It reproduces in the screen with Hide on Scroll
| }); | ||
|
|
||
| const [height, setHeight] = useState(0); | ||
| const animatedValue = useSharedValue(animationType === 'fade' && visible ? 1 : 0); |
There was a problem hiding this comment.
When animationType='slide' and visible=false, the initial animatedValue is 0 — but for slide, hidden state means animatedValue = height. Since height starts at 0, the footer will briefly flash visible until the first layout fires and useEffect updates the value. This is likely a subtle flash-of-content bug on mount.
You can see it in the FloatingButtonScreen, change visible initial value to false and add animationType='slide'
There was a problem hiding this comment.
To fix this we'll probably need a bigger change like I've mentioned in the PR description.
#3987 by Ziv might fix it but it's riskier (bigger regression chance) IMO
2 participants
Description
ScreenFooter - add animationType
Review each commit separately
Note:
There was another solution created by Ziv: #3987 however I feel like it is more bug prone; and even though this solution requires the user to change the animation from
slidetofade(approved by UX) it is overall safer.Changelog
ScreenFooter - add animationType ['slide' (default) | 'fade' | 'none']
Additional info
Ticket 5037