-
-
Notifications
You must be signed in to change notification settings - Fork 533
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
feat(iOS): Add support for UINavigationBackButtonDisplayMode #2123
feat(iOS): Add support for UINavigationBackButtonDisplayMode #2123
Conversation
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.
Great job 🎉 I've left few comments, please answer them.
Example/ios/Podfile.lock
Outdated
@@ -435,9 +435,10 @@ PODS: | |||
- RCT-Folly (= 2021.07.22.00) | |||
- React-Core | |||
- ReactCommon/turbomodule/core | |||
- RNScreens (3.31.0-rc.1): | |||
- RNScreens (3.31.1): |
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 fine, however it would be nice to prepare separate PR with update of podfiles in all examples. Usually we try to not include changes in Podfile.lock & yarn.lock in regular PRs, but this is only a thumb rule.
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.
I'll prepare another PR with bump
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.
And revert this change here.
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.
Bump in #2133
override fun setBackButtonDisplayMode(view: ScreenStackHeaderConfig?, value: String?) { | ||
logNotAvailable("backButtonDisplayMode") | ||
} |
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.
Does it work fine? Because I do not see RNSScreenStackHeaderConfigManagerInterface<ScreenStackHeaderConfig>
being updated.
What we need to do here is run this project on Android Fabric (or just trigger the codegen manually from Android Studio) & copy generated RNSScreenStackHeaderConfigManagerInterface<ScreenStackHeaderConfig>
interface from build folders android/build/generate/source/codegen/java/com/facebook/react/viewmanagers/...
to android/src/paper/java/com/facebook/react/viewmanagers/
so that the interfaces are up to date.
TODO: We need to automate this somehow.
ios/RNSScreenStackHeaderConfig.mm
Outdated
auto isBackButtonCustomized = !isBackTitleBlank || config.disableBackButtonMenu; | ||
auto overrideBackButtonItem = | ||
config.disableBackButtonMenu; // when config.disableBackButtonMenu is true we need to set backButtomItem | ||
prevItem.backButtonTitle = resolvedBackTitle; |
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.
Hmm, now we do always set backButtonTitle
of prevItem
🤔 We need to test it thoroughly for regressions. We need to prepare list of tests examples to test before we can process further.
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.
I reverted that change, seems like it could have negative impact on back button menu.
src/native-stack/types.tsx
Outdated
/** | ||
* How the back button behaves by default (when not customized). Available on iOS>=14. | ||
* The following values are currently supported (they correspond to https://developer.apple.com/documentation/uikit/uinavigationitembackbuttondisplaymode?language=objc): | ||
* - "default" – shows given back button title/previous controller title, system default or just icon based on available space | ||
* - "generic" – shows given system default or just icon based on available space | ||
* - "minimal" – shows just an icon | ||
* @platform ios | ||
*/ | ||
backButtonDisplayMode?: ScreenStackHeaderConfigProps['backButtonDisplayMode']; |
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.
We need to also think through how this property interacts with backTitleVisible
prop.
What happens if we set backTitleVisible: false
and set this to default or other options? And the other way around.
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.
At least we should document these interactions between any related props here.
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.
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.
Ok, I actually revert bunch of this changes. We can't use backButtonTitle to set custom label - more details in PR summary.
ff40589
to
c05e689
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.
LGTM! Nice catch with that getDimensionPropValue
🎉
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 great overall 🎉
I've left some cosmetic remarks.
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.
I think we're good to go here 🎉
(wait for CI to pass before merging)
…e-mansion#2123) ## Description ~This PR improves upon software-mansion#2105. software-mansion#2105 allowed to use iOS 14 default back button behavior when label is not provided. This PR allows to modify the behavior by allowing to provide UINavigationButtonBackButtonDisplayMode and enables it for custom text (without style modifications). The main problem is that we used to provide backButtonItem in most of the cases which [disables](https://developer.apple.com/documentation/uikit/uinavigationitem/3656350-backbuttondisplaymode) backButtonDisplayMode.~ This PR adds possibility to customize default behavior of back button using `backButtonDisplayMode` ([UINavigationBackButtonDisplayMode](https://developer.apple.com/documentation/uikit/uinavigationitem/backbuttondisplaymode)) for iOS. :warning: **This modifies only default back button**, when any customization is added (including headerBackTitle) in native part we create custom `RNSUIBarButtonItem` and set it as `backButtonItem`, which [disables](https://developer.apple.com/documentation/uikit/uinavigationitem/3656350-backbuttondisplaymode) `backButtonDisplayMode` behavior. I tried to make it work together with custom label (`headerBackTitle`) using `prevItem.backButtonTitle`, but due to iOS limitations it is not viable option. It influences also back button menu - changes the label of previous screen - which is not the behavior we want. To sum up, `backButtonDisplayMode` work when none of: - `headerBackTitleStyle.fontFamily` - `headerBackTitleStyle.fontSize` - `headerBackTitle` - `disableBackButtonMenu` are set. ## Screenshots / GIFs |Paper|Fabric| |-|-| |<video src="https://github.com/software-mansion/react-native-screens/assets/11800297/c6aa7697-4331-4cb4-a81d-7f77f128513d" />|<video src="https://github.com/software-mansion/react-native-screens/assets/11800297/fa0edd92-1aa2-45e5-a466-516c0ec120d2" />| <details> <summary>Example component used in tests:</summary> ```jsx import * as React from 'react'; import { Button, View, Text, StyleSheet } from 'react-native'; import { NavigationContainer, ParamListBase } from '@react-navigation/native'; import { createNativeStackNavigator } from '@react-navigation/native-stack'; import { NativeStackNavigationProp } from '@react-navigation/native-stack'; const Stack = createNativeStackNavigator(); type NavProp = { navigation: NativeStackNavigationProp<ParamListBase>; }; export default function App() { return ( <NavigationContainer> <Stack.Navigator> <Stack.Screen name="screenA" component={ScreenA} options={{ headerTitle: 'A: Home' }} /> <Stack.Screen name="screenB" component={ScreenB} options={{ headerTitle: 'B: default', backButtonDisplayMode: 'default', }} /> <Stack.Screen name="screenC" component={ScreenC} options={{ headerTitle: 'C: generic', backButtonDisplayMode: 'generic', }} /> <Stack.Screen name="screenD" component={ScreenD} options={{ headerTitle: 'D: minimal', backButtonDisplayMode: 'minimal', }} /> <Stack.Screen name="screenE" component={ScreenE} options={{ headerTitle: 'E: custom', headerBackTitle: 'Back Title', backButtonDisplayMode: 'minimal', }} /> </Stack.Navigator> </NavigationContainer> ); } const ScreenA = ({ navigation }: NavProp) => ( <View style={styles.container}> <Text>Screen A</Text> <Button onPress={() => navigation.navigate('screenB')} title="Go to screen B" /> </View> ); const ScreenB = ({ navigation }: NavProp) => ( <View style={styles.container}> <Text>Screen B</Text> <Text>backButtonDisplayMode: default</Text> <Button onPress={() => navigation.navigate('screenC')} title="Go to screen C" /> </View> ); const ScreenC = ({ navigation }: NavProp) => ( <View style={{ flex: 1, paddingTop: 50 }}> <Text>Screen C</Text> <Text>backButtonDisplayMode: generic</Text> <Button onPress={() => navigation.navigate('screenD')} title="Go to screen D" /> </View> ); const ScreenD = ({ navigation }: NavProp) => ( <View style={styles.container}> <Text>Screen D</Text> <Text>backButtonDisplayMode: minimal</Text> <Button onPress={() => navigation.navigate('screenE')} title="Go to screen E" /> </View> ); const ScreenE = (_props: NavProp) => ( <View style={styles.container}> <Text>Screen E</Text> <Text>backButtonDisplayMode omitted because of the headerBackTitle</Text> </View> ); const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'space-around' }, }); ``` </details> ## Checklist - [x] Included code example that can be used to test this change - [x] Updated TS types - [x] Updated documentation: <!-- For adding new props to native-stack --> - [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [x] Ensured that CI passes Tested software-mansion#1864: Paper ✅ Fabric ✅ Tested software-mansion#1646: Paper ❌ Fabric ❌ - but it does not work on main too, could now be achieved using `backButtonDisplayMode: ‘minimal’` --------- Co-authored-by: Kacper Kafara <[email protected]>
Description
This PR improves upon #2105. #2105 allowed to use iOS 14 default back button behavior when label is not provided. This PR allows to modify the behavior by allowing to provide UINavigationButtonBackButtonDisplayMode and enables it for custom text (without style modifications). The main problem is that we used to provide backButtonItem in most of the cases which disables backButtonDisplayMode.This PR adds possibility to customize default behavior of back button using
backButtonDisplayMode
(UINavigationBackButtonDisplayMode) for iOS.RNSUIBarButtonItem
and set it asbackButtonItem
, which disablesbackButtonDisplayMode
behavior.I tried to make it work together with custom label (
headerBackTitle
) usingprevItem.backButtonTitle
, but due to iOS limitations it is not viable option. It influences also back button menu - changes the label of previous screen - which is not the behavior we want.To sum up,
backButtonDisplayMode
work when none of:headerBackTitleStyle.fontFamily
headerBackTitleStyle.fontSize
headerBackTitle
disableBackButtonMenu
are set.
Screenshots / GIFs
backButtonDisplayMode.Paper.mov
backButtonDisplayMode.Fabric.mov
Example component used in tests:
Checklist
Tested #1864: Paper ✅ Fabric ✅
Tested #1646: Paper ❌ Fabric ❌ - but it does not work on main too, could now be achieved using
backButtonDisplayMode: ‘minimal’