software-mansion / react-native-screens

Native navigation primitives for your React Native app.
https://docs.swmansion.com/react-native-screens/
MIT License
3.03k stars 514 forks source link

Wrong icon color when push modal on iOS #767

Closed blueberry6401 closed 3 years ago

blueberry6401 commented 3 years ago

Description

When form sheet or page sheet modal is pushed, info icon on status bar should change color to white as shown

Screenshots

RNC Right
ezgif-6-a1f17d5dd8c6 ezgif-6-dfd7927aed0c

Package versions

WoLewicki commented 3 years ago

You can control the the color of the status bar icons with a prop: statusBarStyle. Does it solve your issue?

MoOx commented 3 years ago

I would have expected this to work out of the box. That's weird for a DX point of view. I started to use native-screens to have a more native experience and have less to worry about and would have expected to have this handled as this seems trivial (formSheet on iOS => status bar light when there is more than one screen, am I right?).

I guess if that's not handled, it's because iOS doesn't handle this built-in? Can we add this in this library to have this built-in?

MoOx commented 3 years ago

Because how can we know what color to use? If I have a screen that's white and pushed first, I need dark status bar. But what if this screen is pushed after? I will need light and I cannot really no, or am I missing something? react-navigation stack (js implementation) handle this if I am correct.

MoOx commented 3 years ago

Also we don't know (or don't want to make the test manually) the version of iOS we runs on, and so we don't know if formSheet exist (ex: I am testing on an iPhone 5c and there is no "formSheet" effect, so I don't really want to have a very complexe statusBarStyle value based on OS and position in the stack, when it could be done in this library.

I can help with the right pointers :)

WoLewicki commented 3 years ago

Can you check if #798 fixes all of the problems and does not introduce any new bugs?

MoOx commented 3 years ago

Sorry I don't have any bandwidth for this until next week, meanwhile I used js implementation because I got too much bugs with this native one (mostly navigate() call that were doing nothing and no error)

WoLewicki commented 3 years ago

@MoOx could you submit issues with all of the other bugs that you spotted? It would for sure help with resolving them.

MoOx commented 3 years ago

I kept my work in a branch and want to be sure to reproduce before opening issues, I will try that next friday as I really would like to have use this native module :)

MoOx commented 3 years ago

@WoLewicki I am currently trying master with your included fix, it seems great!

That said, I have some custom headers that didn't use safe area in the past (because formSheet was on all platform, since I used the JS implementation) but now that formSheet is only on iPhone X+ devices, how can I know if I should add my safe area padding? This will be clear with images :)

IMG_E4975

IMG_E4974

I guess, could we have a static method to know if formSheet is supported?

WoLewicki commented 3 years ago

IIRC, the new presentation style is available since iOS 13, so I believe you can just use methods provided by react-native to check the current version: https://reactnative.dev/docs/platform-specific-code#detecting-the-ios-version. But adding a method in react-native-screens that does the same under the hood can be a good option too. I am open to merge a PR with this for sure.

MoOx commented 3 years ago

@WoLewicki it's iOS 13 + notch? Because I don't think and iPhone 6s/7/8 will have this even if they run on iOS 14.

WoLewicki commented 3 years ago

I think it is only the matter of the iOS version. Here you have an iPhone 6s simulator with iOS 14.4: image

MoOx commented 3 years ago

I guess you are right :) Sorry for the bad information. I will try to do a PR for this method.

I have currently a simple thing like this (rescript):

let isFormSheetSupported = Platform.os === Platform.ios && versionIos > "13"

(I can do that in JS very easily, don't worry haha)

Will this be ok for you?

Also I have this too

let formSheetStatusBarStyle = (theme, barStyle) =>
  isFormSheetSupported ? #lightContent : statusBarStyle(theme, barStyle)

let formSheetSafeArea = isFormSheetSupported ? false : true

But I guess this can stick to user land.

WoLewicki commented 3 years ago

I think you can make a PR with both of these things and we can move the discussion there to keep it in one place.