react-native-modal / react-native-modal

An enhanced, animated, customizable Modal for React Native.
MIT License
5.43k stars 614 forks source link

backdrop doesn't cover screen on Samsung Galaxy S8 #147

Open dominicrico opened 6 years ago

dominicrico commented 6 years ago

Hey there,

I'm tried to use your modal package. But the backdrop doesn't cover the whole screen on a Samsung Galaxy S8 when the bottom software button bar is hidden. The react-native package is working correct as of the latest version.

Using latest version of this module and react-native on a Samsung Galaxy S8 with latest software.

Thank you in advance!

mmazzarolo commented 6 years ago

I don’t have any S8 available for testing but I guess it caused by the wrong dimensions reported by the Platfom API of React-Native on your device. Could you report here what’s the result of getting your device height using RN Platform API? Thanks!

dominicrico commented 6 years ago

That is definitely causing the issue. The Dimensions-API always returns the height of the screen with the software bar open. react-native-extra-dimensions-android fixes that problem.

On 21. Apr. 2018, 18:48 +0200, Matteo Mazzarolo notifications@github.com, wrote:

I don’t have any S8 available for testing but I guess it caused by the wrong dimensions reported by the Platfom API of React-Native on your device. Could you report here what’s the result of getting your device height using RN Platform API? Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

vadermemo commented 6 years ago

@dominicrico Can you share the source code of your solution?

Yousefjb commented 6 years ago

Looks like it happens to any device that can display bottom navigation bar but user choose to hide it from android settings

wkoutre commented 6 years ago

@dominicrico I appreciate you sharing the solution of react-native-extra-dimensions-android. I’ve run into this recently with react-native-modal (and the Modal primitive) on an app I’m working on and, lo and behold, this issue (with your comment) was waiting in my inbox this morning!

I’ll go ahead and implement later today, reporting back if I run into anything unusual with extra-dimensions package.

Cheers!

dominicrico commented 6 years ago

@wkoutre thanks for your update. Looking forward to your solution of this issues.

wkoutre commented 6 years ago

@dominicrico react-native-extra-dimensions-android worked excellently. Concise but useful API for the few different "special" Android UI items. Thanks!

Edit: For anyone's reference, I'm doing something like this in a StylesConfig file:

export const IOS = Platform.OS === "ios";
export const WIDTH = Dimensions.get("window").width;
export const HEIGHT = IOS
  ? Dimensions.get("window").height
  : require("react-native-extra-dimensions-android").get("REAL_WINDOW_HEIGHT");

Using an inline require so the react-native-extra-dimensions-android doesn't warn about importing/requiring on iOS.

mmazzarolo commented 6 years ago

Mind adding the info to the FAQs in the README.md? 🙏 thanks!

wkoutre commented 6 years ago

@mmazzarolo Absolutely, sir! Adding now.

mmazzarolo commented 6 years ago

Thanks everyone!

dominicrico commented 6 years ago

@wkoutre thanks for fixing!

wkoutre commented 6 years ago

👍🏼 for sure , team effort!

metahexane commented 6 years ago

Wouldn't close this issue yet, since the solution doesn't work for projects using CRNA, without having it ejected.

mmazzarolo commented 6 years ago

@metahexane are you aware of any way to correctly measure the device height on Expo/CRNA?

metahexane commented 6 years ago

@mmazzarolo I tried to google for it, couldn't find any information. Do you have anything for me?

mmazzarolo commented 6 years ago

Nope, I'm sorry 😞that's why I was asking

metahexane commented 6 years ago

@mmazzarolo I tried changing 'window' to 'screen', such that I have Dimensions.get('screen').height. I'm debugging on the Samsung Galaxy S9, however, the modal goes right under the soft bottom bar, even if I add 1000 to the height, so whether it's the correct height or not, it doesn't matter, the modal always goes under where the soft bottom bar is supposed to be (mine is disabled, so it goes down when it's not in use).

mmazzarolo commented 6 years ago

goes right under the soft bottom bar, even if I add 1000 to the height, so whether it's the correct height or not, it doesn't matter, the modal always goes under where the soft bottom bar

What do you mean by "under the bottom bar"? Isn't it the behaviour you expect from it?

metahexane commented 6 years ago

@mmazzarolo It's like this:

image

As you can see, the modal goes right under the area of the soft bottom bar, while it's not even there.

mmazzarolo commented 6 years ago

🤔 Could you also attach a screenshot with the modal closed?

metahexane commented 6 years ago

This is my view with the model closed (don't mind my poor styling):

image

If I use React's built in Modal, along with flex: 1, I get the result I would want, however animating that model didn't work out for me, so I thought, what if I combine the two modals: React's modal for the background only and react-native-modal's Modal for the animation and main view, however, I had a weird result: the react-native-modal's Modal made React's Modal disappear for some reason and I got the same result as in the first screenshot.

mmazzarolo commented 6 years ago

Could you try changing this line from the react-native-modal folder in node_modules to height: 1000, refresh the app, and see if the backdrop covers the entire screen?

metahexane commented 6 years ago

@mmazzarolo Hey that actually worked for me, wouldn't it be wiser to use Dimensions.get('screen').height in that case? Since screen/window does matter for android and as a model you would actually want to cover the whole screen?

On a side note: does code changed in the node_modules folder get included in the exported apk/ipa, or is the actual source used?

EDIT: It DOES work sometimes (???) if I have the height set to either 1000 or Dimensions.get('screen').height, however, sometimes it gives me the same result as the first screenshot. This behaviour is really weird.

mmazzarolo commented 6 years ago

@mmazzarolo Hey that actually worked for me, wouldn't it be wiser to use Dimensions.get('screen').height in that case? Since screen/window does matter for android and as a model you would actually want to cover the whole screen?

I considered it in the past but measuring the screen was not consistent at the time...

It DOES work sometimes (???) if I have the height set to either 1000 or Dimensions.get('screen').height, however, sometimes it gives me the same result as the first screenshot. This behaviour is really weird.

...And it still seems to be the inconsistent 😞

On a side note: does code changed in the node_modules folder get included in the exported apk/ipa, or is the actual source used?

Yes, it does (if you don't run npm install again before building the apk o ipa.

metahexane commented 6 years ago

...And it still seems to be the inconsistent 😞

It's also inconsistent with height set to 1000. React's built in modal stays consistent...

mike-niemand commented 6 years ago

@wkoutre Mind sharing how you got this to work? I have 'react-native-extra-dimensions-android' installed and working but how is the HEIGHT passed to 'react-native-modal'? Must be missing something.

wkoutre commented 6 years ago

@mike-niemand You've checked out the FAQ section in the README?

If not, check it out. Then apply a style to the style prop of the Modal including the height returned from require("react-native-extra-dimensions-android").get("REAL_WINDOW_HEIGHT").

mike-niemand commented 6 years ago

@wkoutre Yeah, tried that and it made no difference. I am getting the extra dimension height correctly on an S8 but can still see the status bar on react-native-modal (full screen app). Am I correct in assuming that the status bar shouldn't appear?

mmazzarolo commented 6 years ago

Reopening

myarete commented 6 years ago

Also wondering where to put the constants @wkoutre mentioned that caused @mmazzarolo to reopen

jamesreilly5 commented 5 years ago

@mike-niemand @mmazzarolo

Had the same problem. I did some digging into the source and found the issue. The code uses React Native's modal and there's a bug related to this on Android. I solved the problem by implementing the solution in this comment. Note that if you're using a custom splash theme in there you'll probably need to apply the parent to that too (I had to before I got it working anyway)

<resources>

  <!-- Base application theme. -->
  <style name="AppTheme" parent="Theme.ReactNative.AppCompat.Light.NoActionBar.FullScreen">
    <!-- Customize your theme here. -->
  </style>
  <style name="SplashTheme" parent="Theme.ReactNative.AppCompat.Light.NoActionBar.FullScreen">
    <item name="android:windowBackground">@drawable/splash_screen</item>
    <item name="android:statusBarColor">@color/white</item>
  </style>

</resources>
mike-niemand commented 5 years ago

Isn't this corrected in 0.56? I read that it was but can't confirm yet due to the metro bundling issue on Windows.

On Tue, 24 Jul 2018, 09:47 James Reilly, notifications@github.com wrote:

Had the same problem. I did some digging into the source and found the issue. The code uses React Native's modal and there's a bug https://github.com/facebook/react-native/issues/7474 related to this on Android. I solved the problem by implementing the solution in this comment https://github.com/facebook/react-native/issues/7474#issuecomment-288814366. Note that if you're using a custom splash theme in there you'll probably need to apply the parent to that too (I had to before I got it working anyway)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/react-native-community/react-native-modal/issues/147#issuecomment-407313798, or mute the thread https://github.com/notifications/unsubscribe-auth/AZXMq2QBOBP22BI94A2w2fVo_xfHQxWjks5uJtEOgaJpZM4TdlHn .

metahexane commented 5 years ago

@jamesreilly5 that solution won't work for those who haven't ejected.

wkoutre commented 5 years ago

@MrChristofferson I, personally, organize all style-related constants in a assets/CommonStyles.js file.

export const WIDTH = Dimensions.get("window").width;
export const HEIGHT = IOS
  ? Dimensions.get("window").height
  : require("react-native-extra-dimensions-android").get("REAL_WINDOW_HEIGHT");

Then I'm able to import from anywhere. Important to note: My apps are usually portrait-only; therefore, I don't have to worry about the HEIGHT changing.

Without building older projects to be 100% certain of the cleanest working configuration I've implemented, I'd try variations of the style prop on the RNModal imported via import RNModal from "react-native-modal";

Referencing react-native-modal's index.js source, there's:

1) An abolsutely positioned View responsible for onBackdropPress 2) The containerView, which uses the style prop passed into RNModal, either independent or as a child of KeyboardAvoidingView, depending on the usage of the avoidKeyboard prop.

This containerView has styling of:

styles.content = {
    flex: 1,
    justifyContent: "center"
};

const computedStyle = [
      { margin: deviceWidth * 0.05, transform: [{ translateY: 0 }] },
      styles.content,
      style
    ];

So, based on this, there will likely be conflicts when passing in a custom style prop to RNModal. Personally, I shy away from explicitly adding, by default, custom styles to reusable-components because it potentially adds side-effects to its API.

JJMoon commented 5 years ago

I had this issue with Modal in RN. Not react-native-modal. I tried r-n-modal, r-n-extra-dimensions-android in vain. But, I realized that sometime the modal covers full area, and others not working. I guess, the system didn't just once recognizes the full area, but continuously correct the effective area. So, I changed the code from

constructor() {
  this.state = { showModal: true };
}

to

constructor() {
  this.state = { showModal: false };
  setTimeout(() => {
      this.setState({ showModal: true });
    }, 10);
}

And it works.. At last.

I hope this might help. And I use Modal from 'react-native' ...

mmazzarolo commented 5 years ago

I also added a comment on [this PR that might be useful if anyone is willing to give it a test](https://github.com/react-native-community/react-native-modal/pull/208#pullrequestreview-154524379(

mike-niemand commented 5 years ago

Note that this is no longer an issue with React Native 0.57RC

On Wed, Sep 12, 2018 at 9:58 AM, Matteo Mazzarolo notifications@github.com wrote:

I also added a comment on [this PR that might be useful if anyone is willing to give it a test](#208 (review) https://github.com/react-native-community/react-native-modal/pull/208#pullrequestreview-154524379

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/react-native-community/react-native-modal/issues/147#issuecomment-420550642, or mute the thread https://github.com/notifications/unsubscribe-auth/AZXMqwc-GGrlOXRY02hSdyWu8hsV45DVks5uaL6_gaJpZM4TdlHn .

--

CC No: 2009/222342/23

Mike Niemand +2776 7921547

Ground Floor Omnicom Building 66 Peter Place Bryanston Johannesburg South Africa

mmazzarolo commented 5 years ago

Woah, thanks for the update @mike-niemand ! 🎉

adcuz commented 5 years ago

Still evident for me on 0.57.1.

mike-niemand commented 5 years ago

Still evident for me on 0.57.1.

Hmmm. I'm running 0.57.0 and it's fine. I'll try an upgrade to 0.57.1 later and see if the issue is reintroduced.

yasir-netlinks commented 5 years ago

Hi guys, I am having this issue too on S9+, the solution didn't help actually

const IOS = Platform.OS === "ios";
const WIDTH = Dimensions.get("window").width;
const HEIGHT = IOS
  ? Dimensions.get("window").height
  : require("react-native-extra-dimensions-android").get("REAL_WINDOW_HEIGHT");

<Modal
          isVisible={this.state.showFilter}
          animationIn="slideInRight"
          animationOut="slideOutRight"
          onBackdropPress={() => this.setState({ showFilter: false })}
          hideModalContentWhileAnimating={true}
          useNativeDriver={true}
          style={{
            justifyContent: "flex-start",
            alignItems: "flex-end",
            margin: 0,
            height: HEIGHT,
            marginTop: Platform.OS === 'android' ? 54 : 64

          }}
        >
// content
</Modal>

I'm running 0.57.1 Any news regarding this! thank you

mmazzarolo commented 5 years ago

@yasir-netlinks could you try #209 ?

yasir-netlinks commented 5 years ago

@mmazzarolo unfortunately, it didn't work, I am getting HEIGHT value, but nothing changes

<Modal
          isVisible={this.state.showFilter}
          animationIn="slideInRight"
          animationOut="slideOutRight"
          onBackdropPress={() => this.setState({ showFilter: false })}
          hideModalContentWhileAnimating={true}
          useNativeDriver={true}
          style={{
            justifyContent: "flex-start",
            alignItems: "flex-end",
            margin: 0,
            // height: HEIGHT,
            marginTop: Platform.OS === 'android' ? 54 : 64

          }}
          deviceHeight={HEIGHT}
        >
mmazzarolo commented 5 years ago

Can you check that the HEIGHT value is correct? Is react-native-extra-dimensions-android returning the correct height?

yasir-netlinks commented 5 years ago

@mmazzarolo Im not sure how to check, but I did compare it with Dimensions.get('window').height. HEIGHT has a greater value than Dimensions.get('window').height so I am assuming its returning the correct value

mmazzarolo commented 5 years ago

@yasir-netlinks could you also pass the deviceWidth to the modal?

yasir-netlinks commented 5 years ago

@mmazzarolo I added that too, but no luck. Actually, no matter what value I input , no changes take place at all.

mmazzarolo commented 5 years ago

🤔 weird, are you sure you're on the https://github.com/react-native-community/react-native-modal/tree/deviceSizesProps branch?

yasir-netlinks commented 5 years ago

@mmazzarolo ohh it seems im not , is this different from the main one, and if so , how can I install this branch ? I did this only npm i --save react-native-modal@latest

mmazzarolo commented 5 years ago

No worries! I'm still waiting for testers before merging it :)

Following this guide the command should be: npm install --save react-native-community/react-native-modal#deviceSizesProps (also, make sure to run npm uninstall react-native-modal before running it)