-
-
Notifications
You must be signed in to change notification settings - Fork 606
fix(Android, FormSheet): Fix dynamic height change for fitToContents #3484
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
base: main
Are you sure you want to change the base?
Conversation
|
NOW it's feels like Christmas. 🎉 |
kligarski
left a comment
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.
Looks clean, I'm only worried about interactions with different animation system.
Some comments to the assumptions section:
Visual styles such as backgroundColor should be applied via contentStyle on the Screen level. In fitToContents mode, the height of the ScreenContentWrapper should always match the height of the content.
Something I thought about now is that we won't be able to support background image/gradient then but I guess that's not possible on iOS either and it would require more changes. Maybe something to consider in the future.
Unmounting of components remains the responsibility of the application code. We do not include default enter/exit animations at the component level.
I wonder what would happen if there was a component with animation that would change the height? Would this still work correctly? But this also might be something to handle in the future.
| this.translationY = delta | ||
| this | ||
| .animate() | ||
| .translationY(0f) | ||
| .setDuration(sheetHeightAnimationDuration) | ||
| .withStartAction { | ||
| behavior.useSingleDetent(newHeight) | ||
| requestLayout() | ||
| }.withEndAction { | ||
| onSheetYTranslationChanged() | ||
| }.start() |
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.
Won't using ViewPropertyAnimator interfere with ValueAnimators used to animate enter/exit + IME animations? E.g. what would happen if content size changed during enter animation?
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.
Not sure if this is the use case we should cover. I wouldn't consider such a case, as it would require changing content dynamically during enter/exit animation, what would look strange when the content both slides in/out and jumps
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.
atm, I added a prop to disable this default animation, allowing to handle the content by developer and react only to the Screen size change on our side - having the combination: TextInput with autoFocus + unmounting another component at the same time (which is a pretty wild combo) - we may have some conflict which might be problematic to synchronize: textInput could want to move the formSheet up, and unmounted component might require to move the component down at the same moment
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 is how it works with the custom animation:
https://github.com/user-attachments/assets/99d857a6-a124-4778-b16c-3ea57d78b342
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 is how it works with the default one (this flicker is junky, I described the root cause for it above, but anyway, the final state is 'stable')
https://github.com/user-attachments/assets/e721e7d0-265d-41d2-8abf-41ed78c535c4
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 is how it could look with more advanced custom animator:
Screen.Recording.2025-12-18.at.17.38.04.mov
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.
Therefore, I believe that (at least at the early stage, we can recommend this as the proper approach for handling such cases). I'm okay with continuing the development to synchronize it with our default animation, but as for now, I'd propose to add this support in the separate PR as this one already covers at least a few completely different scenarios. Let me know what do you think about the current state.
kligarski
left a comment
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.
Also, not sure if this is a real concern but I was able to make the form sheet not adapt to content by changing the content quickly:
Screen.Recording.2025-12-18.at.09.04.29.mov
I used TestFormSheet with fitToContents and just added some text at the bottom.
checking.. |
Should be fixed, tested with import React, { useRef, useState } from 'react';
import {
Button,
View,
Text,
StyleSheet,
Animated,
} from 'react-native';
import { NavigationContainer } from '@react-navigation/native';
import {
createNativeStackNavigator,
NativeStackScreenProps,
} from '@react-navigation/native-stack';
import Colors from '../shared/styling/Colors';
type StackParamList = {
Home: undefined;
FormSheet: undefined;
};
const Stack = createNativeStackNavigator<StackParamList>();
const HomeScreen = ({ navigation }: NativeStackScreenProps<StackParamList>) => (
<View style={styles.screen}>
<Text style={styles.title}>Home Screen</Text>
<Button
title="Open Form Sheet"
onPress={() => navigation.navigate('FormSheet')}
/>
</View>
);
const FormSheetScreen = () => {
const [isTextVisible, setIsTextVisible] = useState(true);
React.useEffect(() => {
const timer = setTimeout(() => {
setIsTextVisible(false);
}, 1000);
return () => clearTimeout(timer);
}, []);
React.useEffect(() => {
const timer = setTimeout(() => {
setIsTextVisible(true);
}, 1016);
return () => clearTimeout(timer);
}, []);
return (
<View style={styles.formSheetContainer}>
<Text style={styles.formSheetTitle}>Form Sheet Content</Text>
{isTextVisible && (
<Text>
Lorem, ipsum dolor sit amet consectetur adipisicing elit. Velit deserunt
qui fugit unde assumenda. Non id facere mollitia sequi aliquid nostrum,
tenetur eligendi quos at natus eius, corporis quidem eaque libero
reiciendis, labore sed minima doloremque temporibus! Veritatis harum
provident molestias incidunt at temporibus quod ipsam totam optio,
quisquam amet quae molestiae. Exercitationem animi facere iure, nobis
nulla repudiandae aut nam natus! Qui, id? Nobis quia recusandae
repellendus illum voluptas, ipsa, doloribus reprehenderit nulla quas
facere eos! Alias, accusantium voluptatibus doloremque unde eius totam
et officia asperiores? Voluptate totam provident, quas inventore
doloribus autem nihil quisquam soluta eum, dolorum nobis.{' '}
</Text>
)}
</View>
);
};
export default function App() {
return (
<NavigationContainer>
<Stack.Navigator>
<Stack.Screen name="Home" component={HomeScreen} />
<Stack.Screen
name="FormSheet"
component={FormSheetScreen}
options={{
presentation: 'formSheet',
sheetAllowedDetents: 'fitToContents',
contentStyle: {
backgroundColor: Colors.YellowLight40,
},
// TODO(@t0maboro) - add `sheetContentDefaultResizeAnimationEnabled` prop here when possible
}}
/>
</Stack.Navigator>
</NavigationContainer>
);
}
const styles = StyleSheet.create({
screen: {
flex: 1,
alignItems: 'center',
justifyContent: 'center',
padding: 20,
},
title: {
fontSize: 24,
marginBottom: 20,
},
formSheetContainer: {
alignItems: 'center',
justifyContent: 'center',
padding: 20,
gap: 20,
},
formSheetTitle: {
fontSize: 20,
fontWeight: 'bold',
marginBottom: 10,
},
text: {
fontSize: 16,
marginBottom: 8,
color: Colors.White,
},
rectangle: {
width: '100%',
backgroundColor: Colors.NavyLight80,
height: 200,
alignItems: 'center',
justifyContent: 'center',
},
input: {
width: 100,
height: 20,
borderColor: Colors.BlueDark100,
borderWidth: 1,
},
}); |
|
Thank you very much for this PR — looking forward to seeing it merged. :) |
Description
This PR introduces improved support for
fitToContentsin BottomSheet by modifying its style toabsoluteWithNoBottom. This style provides a reference to the current top position of the sheet, making it easier to handle dynamic content within FormSheet.Having improved control over
maxHeight(added in previous PRs), we are now able to implement a smooth resizing animation for the BottomSheet.Important
Key assumptions (these should be carefully reviewed)
backgroundColorshould be applied viacontentStyleon the Screen level. In fitToContents mode, the height of the ScreenContentWrapper should always match the height of the content.This PR also introduces a prop that allows overriding the default enter/exit animation, enabling users who use the Animated/Reanimated API to handle animations manually. When the
sheetContentDefaultResizeAnimationEnabledprop is set, we skip performing our resize + translate animation and only adjust the screen size to match the current content.Fixes: #2560
Changes
absoluteWithNoBottomfor androidfitToContentssheetContentDefaultResizeAnimationEnabledproperty for disabling default animationScreenshots / GIFs
Here you can add screenshots / GIFs documenting your change.
You can add before / after section if you're changing some behavior.
Before
before.mov
animation-before.mov
After
after.mov
animated.mov
Test code and steps to reproduce
Test2560
Checklist