search

Talor-A / react-native-message-bar

A notification bar alert displayed at the top of the screen for react-native
MIT License
55 stars 27 forks source link

iPhone X Support #18

Open keith-kurak opened 6 years ago

keith-kurak commented 6 years ago

I was going to submit a PR from this, and am still happy to do so, but I'd probably make some bad assumptions without getting feedback here on if and how to do it first.

Currently, the message bar doesn't account for the notch on iPhone X, so by default, the message will be partially obscured by the notch. If the message is coming from the top, setting viewTopInset to around 24 got it safely under the notch:

screen shot 2017-11-13 at 12 02 21 pm

This could be a viable workaround, except landscape mode doesn't need the additional padding, and maybe some left/right inset is needed for the bar in landscape (not sure, haven't tried yet). And then there's bars coming up from the bottom, which need to be aware of the 34 px tall home bar area.

Anyway, I was thinking that probably the best thing in the long run is to use SafeAreaView - it takes care of things from all directions without having to think about it. However, that requires React Native 0.50.1. I'm guessing Expo 23 will ship by the end of the month, and it seems likely that it would support RN 50, so at that point, expecting folks to adopt RN 50 for proper iPhone X support seems pretty reasonable. It would probably also make sense for such a change to trigger a new major or minor revision number and list RN 50 as a peer dependency, so it's easy to opt out of until folks are ready to upgrade their RN.

Alternatively, it could check for the availability of SafeAreaView and not support iPhone X automatically if it's not available, or implement the necessary paddings manually without SafeAreaView (ugh).

Talor-A commented 6 years ago

wow thanks for letting me know about this, it didn't even cross my mind! unfortunately I don't have access to an iPhone X / iOS simulator for the time being, so I'll leave this open for now (A PR would be appreciated for this reason, too).

keith-kurak commented 6 years ago

Yeah, I should have time in the next few weeks to knock out a PR (it'll be easier for me to work on once Expo 23 is here). In terms of approach, what do you think of going all-in on SafeAreaView, and making the next minor version (e.g., 2.1.x if I get on this quick enough) compatible only with RN 50 or greater? Then folks who aren't on RN 50 yet can stick with 2.0.x. That's easier IMO than trying to implement the correct paddings manually, and avoids the confusion that would come with iPhone X paddings only working if you used the library with RN 50.

Talor-A commented 6 years ago

Yeah I would agree that's probably the best solution.

keith-kurak commented 6 years ago

So, we might actually get the best of both worlds (iPhone X support without requiring RN 50)- there's a JS-only SafeArea view (https://github.com/react-community/react-native-safe-area-view). I think I'll use this because Expo 23 doesn't even have the native SafeAreaView and it also had some unrelated deal-breaker issues for me. Hopefully I can get a PR going in a few weeks- started on it, but in the middle of a big go-live over here.