-
-
Notifications
You must be signed in to change notification settings - Fork 595
fix(iOS): prevent back button icon from "jumping" during pop animation when display mode minimal is set #2800
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
Conversation
f67b64c to
d9beb18
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.
Notes for future us
| #endif | ||
| // This has any effect only in case the `backBarButtonItem` is not set. | ||
| // We apply it before we configure the back item, because it might get overriden. | ||
| prevItem.backButtonDisplayMode = config.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 don't need to wrap it inside ifdefs due to #2799 (min supported iOS version is 15.1 now
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 really don't like the fact, that when backTitleVisible: false the disableBackButtonMenu is ignored.
However I don't see better solution right now. Any attempt to customise backBarButtonItem & hide the back title will introduce these much more jarring bugs.
@maciekstosio, can we update our types & later react-navigation types & documentation?
In our props we need to state which props introduce custom backBarButtonItem & under backButtonDisplayMode we need to state that it has no effect if backBarButtonItem is customised.
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 good, few last remarks. Thanks.
@maciekstosio
native-stack/README.md
Outdated
| Enum value indicating display mode of back button. It is used only when none of: `backTitleFontFamily`, `backTitleFontSize`, `disableBackButtonMenu`, `backTitle` and `backTitleVisible=false` is set. When the button is customized, under the hood we use iOS native `backButtonItem` which overrides `backButtonDisplayMode`. Read more [#2123](https://github.com/software-mansion/react-native-screens/pull/2123). The `backTitleVisible` forces `backButtonDisplayMode: minimal` and omits other values. Read more [#2800](https://github.com/software-mansion/react-native-screens/pull/2800) | ||
|
|
||
| Possible options: |
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.
Let's apply my previous comment here also
src/types.tsx
Outdated
| * - "generic" – show given system generic or just icon based on available space | ||
| * - "minimal" – show just an icon | ||
| * How the back button behaves. It is used only when none of: `backTitleFontFamily`, `backTitleFontSize`, `disableBackButtonMenu`, `backTitle` and `backTitleVisible=false` is set. | ||
| * To read more see [#2800](https://github.com/software-mansion/react-native-screens/pull/2800) and [#2800](https://github.com/software-mansion/react-native-screens/pull/2800). |
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 don't think we should link these here. Previous link to apple docs should stay.
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 not useful when you trigger signature help.
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.
Okay, I think we're good here. I'm gonna proceed & this can land in another beta.
|
Let it be noted that CI fails for unrelated reasons (seems lik slow runner) |
## Description This PR adds tests for #2809, #1589 and #2800. ## Changes The main assumption is to test all permutation of headerBackButtonMenuEnabled, headerBackButtonDisplayMode and content of button (default - taken from previous screen headerTitle, custom from headerBackTitle and styled when there are styles applied). I hope this will help us tracking regression as this part of the code is a bit confusing and caused us problem in the past. Additionally there are added "custom" scenarios that test default default behavior with default text and custom text (if the back button changes it's appearance depending on available space and [headerBackButtonDisplayMode](https://developer.apple.com/documentation/uikit/uinavigationitem/backbuttondisplaymode-swift.property) specification). The some of the last cases are set to failing, and this is known disparity reported in #2809, we plan to fix it soon after, but wonted to make sure we don't cause a regression. **Remark:** I did it in a way that after each test we're returning to the screen with all test cases for 2809, this is due to the fact that scrolling through list of all Test* takes a lot and reloading RN after each test would cause huge overhead. The downside of it is that if any of the test fails if will cascade fail all other. ## Test code and steps to reproduce You can run test with: ``` detox build --configuration ios.sim.debug detox test --configuration ios.sim.debug e2e/issuesTests/Test2809.e2e.ts -l debug ```` ## Checklist - [ ] Ensured that CI passes
…#2867) ## Description / Changes This is continuation of #2860 that proposes small refactor around back button logic. This area is very confusing and in my opinion this is due to flag `shouldUseCustomBackBarButtonItem` that breaks the flow of other if's and causes other props to not work. Not we contained this flag in `if (visible)` which currently is the only case when backButtonItem might be used after the changes from #2800. ## Test code and steps to reproduce see `FabricExample/e2e/issuesTests/Test2809.e2e.ts` and `apps/src/tests/Test2809/index.tsx` ## Checklist - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <[email protected]>
Description
See: react-navigation/react-navigation#12500
for issue description with videos.
We can use the backbuttondisplaymode to hide the backbuttontitle avoiding setting the title to
niland therefore preventing the UIKit buggy back button icon animation.
@maciekstosio:
We agreed to leave
backTitleVisible, so the API doesn't change in a minor version. The expected behavior is thatbackTitleVisible: falsewill work as "kill switch" and ignore other back button properties. WhenbackTitleVisible: trueother properties will work (including backButtonDisplayMode), but note that iOS omitsbackButtonDisplayModewhen custom back button is provided (so when any ofbackTitleFontSize,backTitleFontFamily,headerBackButtonMenuEnabled,headerBackTitleis used).Changes
☝️
Test code and steps to reproduce
You can use
Test1084.headerBackButtonDisplayMode: 'minimal'on the second push screen.headerLargeTitle: trueon the first push screen.How this has been tested
@maciekstosio:
I tested RNScreens properties by modifying props in
react-navigation/packages/native-stack/src/views/useHeaderConfigProps.tsx.I used Test1084, with the first screen having
headerLargeTitle: true.Tests:
When
backTitleVisible: false:When
backTitleVisible: true | undefined:backButtonDisplayMode: minimalworksbackButtonDisplayMode: genericworksbackButtonDisplayMode: defaultworksbackTitleworks and overridesbackButtonDisplayModeheaderBackTitleStyle.fontSizeworks and overridesbackButtonDisplayModeheaderBackButtonMenuEnabledworks and overridesbackButtonDisplayModeChecklist