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.

Sign up for GitHub

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

Jump to bottom

[RNMobile] Use Reanimated in bottom sheet height animation #52563

Merged
merged 10 commits into from
Jul 14, 2023
Merged

[RNMobile] Use Reanimated in bottom sheet height animation #52563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3f5a220
Expose max height properties in `BottomSheetProvider`
fluiddot Jul 11, 2023
634b2fd
Animate bottom sheet's height with Reanimated
fluiddot Jul 11, 2023
00adbb4
Use pixel value when setting fullscreen height
fluiddot Jul 11, 2023
406366b
Rename `heightRef` to `maxHeight`
fluiddot Jul 11, 2023
ed95231
Re-enable `exhaustive-deps` lint rule in `BottomSheetNavigationContai…
fluiddot Jul 12, 2023
4dcc3d4
Avoid setting height using debounce
fluiddot Jul 12, 2023
c8fe621
Add test ID to navigation container component
fluiddot Jul 12, 2023
4e85db6
Mock Reanimated's `now` function
fluiddot Jul 12, 2023
e4eafc2
Update test cases related to bottom sheet height animation
fluiddot Jul 12, 2023
f7dc1fa
Update test snapshots
fluiddot Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...src/mobile/bottom-sheet/bottom-sheet-navigation/bottom-sheet-navigation-context.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { createContext } from '@wordpress/element';
// Navigation context in BottomSheet is necessary for controlling the
// height of navigation container.
export const BottomSheetNavigationContext = createContext( {
currentHeight: 1,
currentHeight: { value: 0 },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This structure matches Reanimated's shared value. I changed the default value to 0 as I feel it's a more appropriate default value.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that 0 seems like a more reasonable default.

That said, my gut tells me moving to 0 may introduce a bug. I was unable to find a direct answer (indirectly not answered), but my hunch is that using 1 as the default may be a cryptic way of ensuring something occurs when first setting the height, but not if the height is set to 0 in the future. I.e. that "something" should not occur if the height comes through as 0, which more likely to organically occur than 1.

Does that make sense?

My fear may be irrational and not worth following, particularly if we do not observe any bugs while testing now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These default values will only be used when using the provider without setting the value prop. AFAIK this is not our case (reference), so similarly to setting an empty function for setHeight below, I used 0 as the empty value for currentHeight.

Regarding the default value to use for the height animation, I agree it's a bit cryptic why we use 1. I haven't checked this but my gut feeling is that, since we need to calculate the layout of the content to set the final height, using a 0 value would prevent the layout calculations to happen.

Copy link
Member

Choose a reason for hiding this comment

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

The impact — or lack thereof — of the default Context value makes sense. Thank you for noting that.

setHeight: () => {},
} );

Expand Down
115 changes: 64 additions & 51 deletions ...components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-container.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
/**
* External dependencies
*/
import { View, Easing } from 'react-native';
import { NavigationContainer, DefaultTheme } from '@react-navigation/native';
import { createStackNavigator } from '@react-navigation/stack';
import Animated, {
Easing,
useAnimatedStyle,
useSharedValue,
withTiming,
} from 'react-native-reanimated';

/**
* WordPress dependencies
*/
import {
useState,
useContext,
useMemo,
useCallback,
Expand All @@ -23,11 +27,11 @@ import { usePreferredColorSchemeStyle } from '@wordpress/compose';
/**
* Internal dependencies
*/
import { performLayoutAnimation } from '../../layout-animation';
import {
BottomSheetNavigationContext,
BottomSheetNavigationProvider,
} from './bottom-sheet-navigation-context';
import { BottomSheetContext } from '../bottom-sheet-context';

import styles from './styles.scss';

Expand Down Expand Up @@ -57,67 +61,73 @@ const options = {
cardStyleInterpolator: fadeConfig,
};

const ANIMATION_DURATION = 190;
const HEIGHT_ANIMATION_DURATION = 300;
const DEFAULT_HEIGHT = 1;
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved

function BottomSheetNavigationContainer( {
children,
animate,
main,
theme,
style,
testID,
} ) {
const Stack = useRef( createStackNavigator() ).current;
const context = useContext( BottomSheetNavigationContext );
const [ currentHeight, setCurrentHeight ] = useState(
context.currentHeight || 1
const navigationContext = useContext( BottomSheetNavigationContext );
const { maxHeight: sheetMaxHeight, isMaxHeightSet: isSheetMaxHeightSet } =
useContext( BottomSheetContext );
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
const currentHeight = useSharedValue(
navigationContext.currentHeight?.value || DEFAULT_HEIGHT
);

const backgroundStyle = usePreferredColorSchemeStyle(
styles.background,
styles.backgroundDark
);

const _theme = theme || {
...DefaultTheme,
colors: {
...DefaultTheme.colors,
background: backgroundStyle.backgroundColor,
},
};
const defaultTheme = useMemo(
() => ( {
...DefaultTheme,
colors: {
...DefaultTheme.colors,
background: backgroundStyle.backgroundColor,
},
} ),
[ backgroundStyle.backgroundColor ]
);
Comment on lines +88 to +97
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is memoized to avoid potentially generating a new object on every render in _theme.

const _theme = theme || defaultTheme;

const setHeight = useCallback(
( height ) => {
// The screen is fullHeight.
if (
typeof height === 'string' &&
typeof height !== typeof currentHeight
height > DEFAULT_HEIGHT &&
Math.round( height ) !== Math.round( currentHeight.value )
) {
performLayoutAnimation( ANIMATION_DURATION );
setCurrentHeight( height );

return;
}

if (
height > 1 &&
Math.round( height ) !== Math.round( currentHeight )
) {
if ( currentHeight === 1 ) {
setCurrentHeight( height );
} else if ( animate ) {
performLayoutAnimation( ANIMATION_DURATION );
setCurrentHeight( height );
// If max height is set in the bottom sheet, we clamp
// the new height using that value.
const newHeight = isSheetMaxHeightSet
? Math.min( sheetMaxHeight, height )
: height;
const shouldAnimate =
animate && currentHeight.value !== DEFAULT_HEIGHT;

if ( shouldAnimate ) {
currentHeight.value = withTiming( newHeight, {
duration: HEIGHT_ANIMATION_DURATION,
easing: Easing.out( Easing.cubic ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can check the easing function here.

} );
} else {
setCurrentHeight( height );
currentHeight.value = newHeight;
}
}
},
// Disable reason: deferring this refactor to the native team.
// see https://github.com/WordPress/gutenberg/pull/41166
// eslint-disable-next-line react-hooks/exhaustive-deps
[ currentHeight ]
[ animate, currentHeight, isSheetMaxHeightSet, sheetMaxHeight ]
);

const animatedStyles = useAnimatedStyle( () => ( {
height: currentHeight.value,
} ) );

const screens = useMemo( () => {
return Children.map( children, ( child ) => {
let screen = child;
Expand All @@ -136,19 +146,16 @@ function BottomSheetNavigationContainer( {
/>
);
} );
// Disable reason: deferring this refactor to the native team.
// see https://github.com/WordPress/gutenberg/pull/41166
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ children ] );
}, [ children, main ] );

return useMemo( () => {
return (
<View style={ [ style, { height: currentHeight } ] }>
<Animated.View
style={ [ style, animatedStyles ] }
testID={ testID }
>
<BottomSheetNavigationProvider
value={ {
setHeight,
currentHeight,
} }
value={ { setHeight, currentHeight } }
>
{ main ? (
<NavigationContainer theme={ _theme }>
Expand All @@ -168,12 +175,18 @@ function BottomSheetNavigationContainer( {
</Stack.Navigator>
) }
</BottomSheetNavigationProvider>
</View>
</Animated.View>
);
// Disable reason: deferring this refactor to the native team.
// see https://github.com/WordPress/gutenberg/pull/41166
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ currentHeight, _theme ] );
}, [
_theme,
animatedStyles,
currentHeight,
main,
screens,
setHeight,
style,
testID,
] );
}

export default BottomSheetNavigationContainer;
36 changes: 15 additions & 21 deletions ...es/components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-screen.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@ import {
useNavigation,
useFocusEffect,
} from '@react-navigation/native';
import { View, ScrollView, TouchableHighlight } from 'react-native';
import {
ScrollView,
TouchableHighlight,
useWindowDimensions,
View,
} from 'react-native';

/**
* WordPress dependencies
*/
import { BottomSheetContext } from '@wordpress/components';
import { debounce } from '@wordpress/compose';
import { useRef, useCallback, useContext, useMemo } from '@wordpress/element';

/**
Expand All @@ -29,7 +33,7 @@ const BottomSheetNavigationScreen = ( {
name,
} ) => {
const navigation = useNavigation();
const heightRef = useRef( { maxHeight: 0 } );
const maxHeight = useRef( 0 );
const isFocused = useIsFocused();
const {
onHandleHardwareButtonPress,
Expand All @@ -38,16 +42,10 @@ const BottomSheetNavigationScreen = ( {
listProps,
safeAreaBottomInset,
} = useContext( BottomSheetContext );
const { height: windowHeight } = useWindowDimensions();

const { setHeight } = useContext( BottomSheetNavigationContext );

// Disable reason: deferring this refactor to the native team.
// see https://github.com/WordPress/gutenberg/pull/41166
// eslint-disable-next-line react-hooks/exhaustive-deps
const setHeightDebounce = useCallback( debounce( setHeight, 10 ), [
setHeight,
] );

useFocusEffect(
useCallback( () => {
onHandleHardwareButtonPress( () => {
Expand Down Expand Up @@ -81,28 +79,24 @@ const BottomSheetNavigationScreen = ( {
useFocusEffect(
useCallback( () => {
if ( fullScreen ) {
setHeight( '100%' );
setHeight( windowHeight );
setIsFullScreen( true );
} else if ( heightRef.current.maxHeight !== 0 ) {
} else if ( maxHeight.current !== 0 ) {
setIsFullScreen( false );
setHeight( heightRef.current.maxHeight );
setHeight( maxHeight.current );
}
return () => {};
// Disable reason: deferring this refactor to the native team.
// see https://github.com/WordPress/gutenberg/pull/41166
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ setHeight ] )
}, [ fullScreen, setHeight, setIsFullScreen, windowHeight ] )
);

const onLayout = ( { nativeEvent } ) => {
if ( fullScreen ) {
return;
}
const { height } = nativeEvent.layout;

if ( heightRef.current.maxHeight !== height && isFocused ) {
heightRef.current.maxHeight = height;
setHeightDebounce( height );
if ( maxHeight.current !== height && isFocused ) {
maxHeight.current = height;
setHeight( height );
}
};

Expand Down
Loading
Loading