fix: update header in vc below modal #1228
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
7 tasks
|
@WoLewicki Issue seems to be fixed, I thought it wasn't at first but someone pointed out that was due to testing in expo. Thanks a lot for the fix! Much appreciated!! |
2 tasks
8 tasks
kkafar
pushed a commit
that referenced
this pull request
Jul 5, 2024
…2229) ## Description When we push multiple screens with different configuration of `headerShown` prop (e.g. 4 screens with visible header and one screen with hidden header), open a modal and trigger rerenders on screens (e.g. by changing the theme) there's a high chance that value of header hide prop will be incorrect on the screen under the modal. Big kudos to @WoLewicki for helping with debugging! ## Changes `isPresentingVC` flag wasn't checking if current view controller was on the top, which was causing header updates for all screens in the stack (moreover it was done in non-deterministic order as context updates can cause non-deterministic rerenders of particular screens), so check `&& vc == nav.topViewController` was added to `isPresentingVC` flag in `ios/RNSScreenStackHeaderConfig.mm` file. After that change, we still update the screen which is directly under the modal, so fix merged in #1228 is still valid. ## Screenshots / GIFs ### Before https://github.com/software-mansion/react-native-screens/assets/44415389/cf9b07fc-4e95-4362-95c7-ab387a94148e ### After https://github.com/software-mansion/react-native-screens/assets/44415389/94a5dd72-498b-4959-a2de-1e6008d63895 ## Test code and steps to reproduce 1. `Test2229` was added, so just render it in `TestsExample` app 2. Push a few screens with header 3. Push screen without header 4. Open modal 5. Background and open the app or change theme 6. Close the modal 7. Before the fix header should be visible on the screen although it should be a screen without a header ## Checklist - [X] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…oftware-mansion#2229) ## Description When we push multiple screens with different configuration of `headerShown` prop (e.g. 4 screens with visible header and one screen with hidden header), open a modal and trigger rerenders on screens (e.g. by changing the theme) there's a high chance that value of header hide prop will be incorrect on the screen under the modal. Big kudos to @WoLewicki for helping with debugging! ## Changes `isPresentingVC` flag wasn't checking if current view controller was on the top, which was causing header updates for all screens in the stack (moreover it was done in non-deterministic order as context updates can cause non-deterministic rerenders of particular screens), so check `&& vc == nav.topViewController` was added to `isPresentingVC` flag in `ios/RNSScreenStackHeaderConfig.mm` file. After that change, we still update the screen which is directly under the modal, so fix merged in software-mansion#1228 is still valid. ## Screenshots / GIFs ### Before https://github.com/software-mansion/react-native-screens/assets/44415389/cf9b07fc-4e95-4362-95c7-ab387a94148e ### After https://github.com/software-mansion/react-native-screens/assets/44415389/94a5dd72-498b-4959-a2de-1e6008d63895 ## Test code and steps to reproduce 1. `Test2229` was added, so just render it in `TestsExample` app 2. Push a few screens with header 3. Push screen without header 4. Open modal 5. Background and open the app or change theme 6. Close the modal 7. Before the fix header should be visible on the screen although it should be a screen without a header ## Checklist - [X] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added a check for if we are in vc below modal since then we also want to update header as it is visible. Should resolve #1225.
Screenshots / GIFs
You can see them in #1225.
Test code and steps to reproduce
Test1228.tsx(setenableFreeze(false);inApp.jsto test it since we want updates in screen below).Checklist