-
-
Notifications
You must be signed in to change notification settings - Fork 588
fix(iOS, Stack): Fix calculating insets for header title #3120
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
It looks like they're using SwiftUI to implement UIKit xD |
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.
Few comments & questions
return totalMargins; | ||
} | ||
|
||
UIView *currentView = titleControl.superview; |
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.
On the snippet you provided in PR description I can observe, that titleControl
has non-zero insets. Why do we skip them by starting from superview?
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'm following the same approach as for the < iOS26
implementation for which we stopped at _UINavigationBarTitleControl
level
while (currentView && currentView != self) { | ||
NSDirectionalEdgeInsets margins = currentView.directionalLayoutMargins; | ||
totalMargins.top += margins.top; | ||
totalMargins.leading += margins.leading; | ||
totalMargins.bottom += margins.bottom; | ||
totalMargins.trailing += margins.trailing; | ||
currentView = currentView.superview; | ||
} |
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 loop also does not include insets of UINavigationBar
itself, is it intentional?
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.
NavigationBar was left in their old calculation method: https://github.com/software-mansion/react-native-screens/blob/main/ios/RNSScreenStackHeaderConfig.mm#L256. For this change, I intentionally wanted to calculate everything in between UINavigationBar and title control view.
74d7168
to
acdc220
Compare
This PR introduces a regression with shrinking title component in landscape mode too much. I'm working on another approach for calculating edge insets. Closing this in favor of: #3210 |
Description
This PR is changing the approach for calculating edge insets of the navigation bar.
On iOS 26, the hierarchy has changed significantly, resulting in placing too long header content, which might be covered by the right button.
Old hierarchy:
New hierarchy:
In the new approach, I'm summing up all
directionalLayoutMargins
between_UINavigationBarTitleControl
andNavigationBarContentView
. I know that this approach doesn't seem to be super-solid, but somehow I need to traverse the structure to get_UINavigationBarTitleControl
and calculate cumulative layout properly.Changes
UINavigationBar
with the new method for calculating the insets between the content and title views.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
TestHeaderTitle
example on iOS 26 and 18.Checklist