fix(iOS): right header incorrect position #2316
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
thomasttvo
reviewed
Aug 23, 2024
| @@ -72,9 +72,6 @@ - (void)updateLayoutMetrics:(const react::LayoutMetrics &)layoutMetrics | |||
| self); | |||
| } else { | |||
| self.bounds = CGRect{CGPointZero, frame.size}; | |||
| // We're forcing the parent view to layout this subview with correct frame size, | |||
| // see: https://github.com/software-mansion/react-native-screens/pull/2248 | |||
| [self.superview layoutIfNeeded]; | |||
There was a problem hiding this comment.
I'm not super familiar with this code, but will this take away the ability to position the layout when the headerRight view changes internally (not through a setOptions)?
useEffect(() => {
navigation.setOptions({
headerRight: () => <HeaderRight />
})
}, [])
const HeaderRight = () => {
const [x, setX] = useState(false)
useTimeout(() => setX(true), 2000)
return <View style={{backgroundColor:'green', height:20, width: x ? 40 : 20}} />
}There was a problem hiding this comment.
@thomasttvo It'll be fine 😉. If you have any concerns, feel free to test this change within your project and let me know if it works as intended.
kkafar
requested changes
Sep 26, 2024
There was a problem hiding this comment.
Hey, native code changes look ok, if you claim this solves the issues. What I want you to do right now is:
- Add some inline comment around the line with
layoutIfNeededyou added explaining why it is there. - Rebase the PR on main & make sure that it still works, since there were some changes interfering with header layout logic since this PR has been opened.
Great job overall!
|
@kkafar Rebased and added a brief comment. I can confirm the fix is still doing it's job 😉 |
|
patched this into our project and fixes our issue with the right header 👍 |
|
@Jonoh89, thanks for confirmation! |
2 tasks
alduzy
added a commit
that referenced
this pull request
Oct 7, 2024
## Description This PR intents to fix header subviews incorrect layout when changing tabs. The previous solution did layout the subviews correctly in the test cases, but triggered an undesirable `layoutIfNeeded` when going back from tab to tab. In such case the navigation layout happened without updating subview's layout metrics. Moving the logic to subview resolves the issue as the re-layout is now triggered only when subview's layout metrics are updated. Related fixes from the past: #2316, #2248 ## Changes - combined `Test2231.tsx` with `Test432.tsx` to create comprehensive test case - moved re-layout logic to subview ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - Use `Test432.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
## Description This PR fixes the incorrect position of the custom header items when updating more than one option at the same time. The proposed solution let us remove the previous fix for a similar problem: software-mansion#2248 which only fixed the issue on fabric. Fixes software-mansion#432, software-mansion#2231. ## Changes - forced re-layout of the navigation controller when subviews are updated - removed previous fix - updated `Test432` repro <!-- ## Screenshots / GIFs Here you can add screenshots / GIFs documenting your change. You can add before / after section if you're changing some behavior. ### Before ### After --> ## Test code and steps to reproduce - use `Test432` and `Test2231` to test this fix on both architectures. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
## Description This PR intents to fix header subviews incorrect layout when changing tabs. The previous solution did layout the subviews correctly in the test cases, but triggered an undesirable `layoutIfNeeded` when going back from tab to tab. In such case the navigation layout happened without updating subview's layout metrics. Moving the logic to subview resolves the issue as the re-layout is now triggered only when subview's layout metrics are updated. Related fixes from the past: software-mansion#2316, software-mansion#2248 ## Changes - combined `Test2231.tsx` with `Test432.tsx` to create comprehensive test case - moved re-layout logic to subview ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - Use `Test432.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] 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
This PR fixes the incorrect position of the custom header items when updating more than one option at the same time. The proposed solution let us remove the previous fix for a similar problem: #2248 which only fixed the issue on fabric.
Fixes #432, #2231.
Changes
Test432reproTest code and steps to reproduce
Test432andTest2231to test this fix on both architectures.Checklist