fix(iOS): change implementation of calculating status bar, refactor methods used on header height change #1917

tboba · 2023-10-11T08:11:14Z

Description

It looks that sometimes when you're most likely on the first screen the initial header height (taken from the getDefaultHeaderHeight method) stays and is not being updated from the onHeaderHeightChange event. Also, when user hides the status bar it wasn't counted to the final value of header height. This PR fixes those problems and also other minor issues, related to the calculating header height.

Changes

Added calls for calculating header height on setting animated config and changing statusBarHidden prop.
Changed implementation of getCalculatedStatusBarHeightIsModal method.
Refactorized naming of the methods, related to the header height.
Added asserting modal hierarchy.

Test code and steps to reproduce

You can change Modals.tsx file by adding this snippet:

    const headerHeight = useAnimatedHeaderHeight();
    headerHeight.addListener((height) => console.log(height.value))

to the components and listen to the changes in Modals example. Then try to hide header in modals - there should be 0 value as a header height.

Checklist

Included code example that can be used to test this change
Ensured that CI passes

…status bar visibility

kkafar

I have left a question below

ios/RNSScreen.mm

tboba · 2023-10-11T09:45:51Z

ios/RNSScreen.mm

 - (CGFloat)calculateHeaderHeightIsModal:(BOOL)isModal
 {
-  CGFloat navbarHeight = [self getCalculatedHeaderHeightIsModal:isModal];
-  CGSize statusBarSize = [self getCalculatedStatusBarHeightIsModal:isModal];
-
-  // Unfortunately, UIKit doesn't care about switching width and height options on screen rotation.
-  // We should check if user has rotated its screen, so we're choosing the minimum value between the
-  // width and height.
-  CGFloat statusBarHeight = MIN(statusBarSize.width, statusBarSize.height);
-  return navbarHeight + statusBarHeight;
+  CGFloat navbarHeight = [self getNavigationBarHeightIsModal:isModal];
+  CGFloat navbarInset = [self getNavigationBarInsetIsModal:isModal];
+  return navbarHeight + navbarInset;
 }

 - (void)calculateAndNotifyHeaderHeightChangeIsModal:(BOOL)isModal


Just one comment from me - leaving the name calculateHeaderHeightIsModal is made on purpose here. I did not change it to calculateNavigationBarHeightIsModal because for me header is the navigationBar + an inset of it - that's why I'm leaving it here.

kkafar

Few more remarks.

Also please update PR title before merging (as events are no longer sent on status bar visibility change)

ios/RNSScreenStackHeaderConfig.mm

kkafar · 2023-10-11T13:44:14Z

ios/RNSScreenStackHeaderConfig.mm

+  // As the header might have change in `updateViewController` we need to ensure that header height
+  // returned by the `onHeaderHeightChange` event is correct.
+  if ([vc isKindOfClass:[RNSScreen class]]) {
+    [(RNSScreen *)vc calculateAndNotifyHeaderHeightChangeIsModal:NO];
+  }


In this case I'm not sure, so we can leave it, but I have a feeling that it is also always RNSScreen.

Let's keep it for now - I'm afraid if there will be some cases that vc won't be actually RNSScreen (RNSScreenStack?) and that may cause some crashes on iOS.

ios/RNSScreen.mm

kkafar

Looks good beside one thing I mention below

ios/RNSScreenStackHeaderConfig.mm

kkafar

LGTM 🎉

tboba · 2023-10-11T17:25:44Z

Hey @kkafar, as I've made some changes (as I discovered a bug while testing modals 😅) and also refactorized even more some places, can you take a final look on the changes? 🙏

kkafar

Hey, I've found the use case I told you about yesterday: if headerShown: false is set on modal it won't have RNSNavigationController in its hierarchy.

Does the code cover this case?
I believe 0 should be returned.

kkafar · 2023-10-13T09:27:09Z

Also after this PR is approved, please update PR description so that it reflects what actually this PR does.

kkafar

Few more remarks. Getting close

ios/RNSScreen.mm

…chy to isModalWithHeader, move statusBarSize var

kkafar

🟢

…ethods used on header height change (software-mansion#1917) ## Description It looks that sometimes when you're most likely on the first screen the initial header height (taken from the `getDefaultHeaderHeight` method) stays and is not being updated from the `onHeaderHeightChange` event. Also, when user hides the status bar it wasn't counted to the final value of header height. This PR fixes those problems and also other minor issues, related to the calculating header height. ## Changes - Added calls for calculating header height on setting animated config and changing `statusBarHidden` prop. - Changed implementation of `getCalculatedStatusBarHeightIsModal` method. - Refactorized naming of the methods, related to the header height. - Added asserting modal hierarchy. ## Test code and steps to reproduce You can change `Modals.tsx` file by adding this snippet: ```js const headerHeight = useAnimatedHeaderHeight(); headerHeight.addListener((height) => console.log(height.value)) ``` to the components and listen to the changes in `Modals` example. Then try to hide header in modals - there should be `0` value as a header height. ## Checklist - [X] Included code example that can be used to test this change - [ ] Ensured that CI passes

Add sending events about header height on setting config and chaning …

83c8abc

…status bar visibility

tboba requested a review from kkafar October 11, 2023 08:11

tboba self-assigned this Oct 11, 2023

tboba added 2 commits October 11, 2023 10:17

Change place of updating header height

2d8592c

Change vc to self.screenView.controller

dda38c3

kkafar reviewed Oct 11, 2023

View reviewed changes

ios/RNSScreen.mm Outdated Show resolved Hide resolved

tboba added 3 commits October 11, 2023 11:09

Change implementation of getCalculatedStatusBarHeightIsModal method

6e3cbb9

Remove calculating header height in statusBarHidden

46364e5

Refactor naming of methods related to header height

9529047

tboba commented Oct 11, 2023

View reviewed changes

tboba requested a review from kkafar October 11, 2023 09:46

kkafar reviewed Oct 11, 2023

View reviewed changes

tboba changed the title ~~fix(iOS): Send events on setting animated config and changing status bar visibility~~ fix(iOS): change implementation of calculating status bar, refactor naming Oct 11, 2023

tboba changed the title ~~fix(iOS): change implementation of calculating status bar, refactor naming~~ fix(iOS): change implementation of calculating status bar, refactor methods used on header height change Oct 11, 2023

Apply changes from code review

8884bef

kkafar reviewed Oct 11, 2023

View reviewed changes

ios/RNSScreenStackHeaderConfig.mm Outdated Show resolved Hide resolved

Remove (RNSScreen *) leftover

40df2fd

kkafar approved these changes Oct 11, 2023

View reviewed changes

tboba added 2 commits October 11, 2023 19:10

Fix issue with not counting navigation bar inset in transparent modals

3d4bd36

Change self.topViewController to screenController

4f45e5f

Change hasModalOpened to assertModalHierarchy, add assertions

9c0a332

kkafar reviewed Oct 13, 2023

View reviewed changes

tboba added 4 commits October 13, 2023 12:50

Add additional checks for calculating header height

e910699

Fix negative values counted for the modals

b0243c7

Add isModal check

dc987c6

Add check if modal is being dismissed to avoid redundant calculations

686a221

kkafar reviewed Oct 17, 2023

View reviewed changes

ios/RNSScreen.mm Outdated Show resolved Hide resolved

ios/RNSScreen.mm Outdated Show resolved Hide resolved

ios/RNSScreen.mm Outdated Show resolved Hide resolved

ios/RNSScreen.mm Show resolved Hide resolved

Remove count == 0 from assertModalHierarchy

e779764

Remove checks for childViewController.count, change assertModalHierar…

add6ee3

…chy to isModalWithHeader, move statusBarSize var

kkafar approved these changes Oct 17, 2023

View reviewed changes

tboba merged commit dbb7430 into main Oct 17, 2023
6 checks passed

tboba deleted the @tboba/header-height-improvements branch October 17, 2023 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iOS): change implementation of calculating status bar, refactor methods used on header height change #1917

fix(iOS): change implementation of calculating status bar, refactor methods used on header height change #1917

tboba commented Oct 11, 2023 •

edited

Loading

kkafar left a comment

tboba Oct 11, 2023 •

edited

Loading

kkafar left a comment

kkafar Oct 11, 2023

tboba Oct 11, 2023

kkafar left a comment

kkafar left a comment

tboba commented Oct 11, 2023 •

edited

Loading

kkafar left a comment

kkafar commented Oct 13, 2023

kkafar left a comment

kkafar left a comment

fix(iOS): change implementation of calculating status bar, refactor methods used on header height change #1917

fix(iOS): change implementation of calculating status bar, refactor methods used on header height change #1917

Conversation

tboba commented Oct 11, 2023 • edited Loading

Description

Changes

Test code and steps to reproduce

Checklist

kkafar left a comment

Choose a reason for hiding this comment

tboba Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

kkafar Oct 11, 2023

Choose a reason for hiding this comment

tboba Oct 11, 2023

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

tboba commented Oct 11, 2023 • edited Loading

kkafar left a comment

Choose a reason for hiding this comment

kkafar commented Oct 13, 2023

kkafar left a comment

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

tboba commented Oct 11, 2023 •

edited

Loading

tboba Oct 11, 2023 •

edited

Loading

tboba commented Oct 11, 2023 •

edited

Loading