Skip to content
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

Reanimated and GestureHandler upgrade #3203

Merged
merged 27 commits into from
Sep 1, 2024
Merged

Conversation

Inbal-Tish
Copy link
Collaborator

@Inbal-Tish Inbal-Tish commented Aug 8, 2024

Description

Reanimated and GestureHandler upgrade.
In preparation for the RN73 upgrade we need to upgrade Reanimated (from 3.4.0 to 3.8.1) and GestureHandler (from 2.9.0 to 2.14.1).

The upgrades involved some migrations:

  1. Wrapping with GestureHandlerRootView - Modal wrap on iOS too.
  2. Wrapping with GestureHandlerRootView when needed - SectionWheelPickerScreen.
  3. Remove ts ignore for a bug fix on Reanimated 3.5.0.
  4. ts-expect-error where useAnimatedGestureHandler migration is needed (creates bug on PanView on Android with this version).

Known issue - PanView gesture does not work on Android only.

Changelog

Upgrading Reanimated (3.8.1) and GestureHandler (2.14.1) + upgrade fixes

Additional info

ticket #4185

@Inbal-Tish Inbal-Tish changed the title GestureHandler upgrade fixes Reanimated and GestureHandler upgrade Aug 8, 2024
@Inbal-Tish Inbal-Tish marked this pull request as draft August 13, 2024 08:37
@Inbal-Tish Inbal-Tish marked this pull request as ready for review August 14, 2024 13:55
Copy link
Contributor

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marquee - iOS - horizontal not working (need to test on master)
TabController - both - no indicator (line beneath the selected tab item; need to test on master)
Slider - Android - performance is really bad (need to test on master)
ColorPickerScreen - both - need to wrap with <GestureHandlerRootView style={styles.container}> (remove the style from ScrollView)
PickerScreen - both - need to wrap with GestureHandlerRootView
PanView - both - drag does not work

Still did not go over everything

@Inbal-Tish
Copy link
Collaborator Author

Marquee - iOS - horizontal not working (need to test on master) TabController - both - no indicator (line beneath the selected tab item; need to test on master) Slider - Android - performance is really bad (need to test on master) ColorPickerScreen - both - need to wrap with <GestureHandlerRootView style={styles.container}> (remove the style from ScrollView) PickerScreen - both - need to wrap with GestureHandlerRootView PanView - both - drag does not work

Still did not go over everything

Marquee - doesn't work on master - works on a real device on both branches.
Slider - works fine on both platforms on master and upgrade branches.
TabController - works fine on both platforms on master, on upgrade branch no indicator.
PanView - Works fine on both platforms on master, on upgrade branch works fine on iOS, not on Android.
PickerScreen, ColorPickerScreen, GridViewScreen - fixed (wrapped with gestureHandlerRootHOC).
GridViewScreen - looks weird on both masted and upgrade branches.
ProgressiveImageScreen - crashes on both masted and upgrade branches.

@M-i-k-e-l M-i-k-e-l assigned ethanshar and unassigned M-i-k-e-l Aug 20, 2024
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M-i-k-e-l
Wrote small comments, anyway approved

Comment on lines +153 to +156
value.forEach((_, index) => {
value[index] = widths[index];
});
return value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's something a little confusing when you both update the value and return it.
Maybe use map instead of forEach and return it, it might even shorten this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it but it caused a TS error I could not find a fix for.

Comment on lines +161 to +165
value.forEach((_, index) => {
if (index > 0) {
value[index] = value[index - 1] + widths[index - 1];
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, consider using map

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that accessing value[index - 1] gives 0

@M-i-k-e-l M-i-k-e-l enabled auto-merge (squash) August 27, 2024 11:34
@M-i-k-e-l M-i-k-e-l merged commit 0aa3ee0 into master Sep 1, 2024
1 check passed
@M-i-k-e-l M-i-k-e-l deleted the fix/GestureHandlerRootView branch September 1, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants