ladjs / react-native-loading-spinner-overlay

:barber: React Native loading spinner overlay
MIT License
1.59k stars 173 forks source link

Spinner stuck if combined with an alert. #82

Closed cristianoccazinsp closed 4 years ago

cristianoccazinsp commented 5 years ago

Following https://github.com/joinspontaneous/react-native-loading-spinner-overlay/issues/61 and https://github.com/joinspontaneous/react-native-loading-spinner-overlay/issues/72

Spinner/modal getting stuck is still an issue that happens more often than one would want. The timeout fix is really not appropriate if you are showing the spinner automatically through redux state changes, or from other components.

On the react native side, they claim to have this issue fixed already.

I wonder if this line is the culprit. Completely removing the modal component before the UI/Native has time to dismiss it might be causing issues? https://github.com/joinspontaneous/react-native-loading-spinner-overlay/blob/master/index.js#L139

Otherwise, any other ideas on how to fix/improve this? I'm even considering adding random delays to the modal dismiss call, but that's definitely ugly as hell.

niftylettuce commented 5 years ago

Did you try removing/changing what happens with L139? Let me know if you fixed it and we can get it patched here. Feel free to submit PR.

cristianoccazinsp commented 5 years ago

I will be testing that later tonight since my current code can't reproduce the issue easily. I was asking mainly to confirm if that wasn't tested already so I don't spend time on it haha.

El jue., 16 de mayo de 2019 15:44, niftylettuce notifications@github.com escribió:

Did you try removing/changing what happens with L139? Let me know if you fixed it and we can get it patched here. Feel free to submit PR.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/joinspontaneous/react-native-loading-spinner-overlay/issues/82?email_source=notifications&email_token=ALU263EV3L7VLFECRATBE3TPVWTSVA5CNFSM4HNNFQD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVSW5NY#issuecomment-493186743, or mute the thread https://github.com/notifications/unsubscribe-auth/ALU263HWLBRKX5KE4FQ45ATPVWTSVANCNFSM4HNNFQDQ .

cristianoccazinsp commented 5 years ago

That didn't work, neither some other stuff I have tried. However, the culprit is really the cancelable flag: https://github.com/joinspontaneous/react-native-loading-spinner-overlay/blob/master/index.js#L111

IOS can't seem to render properly an alert and modal at the same time, so when the new alert fires, the request close method of the modal is also fired. Not handling this close request pretty much makes the modal stuck, but handling it will close the modal defeating its purpose.

My question is, in what other cases does IOS fire the request close event? Not like IOS has a back button? Can we just close and re-open the modal if it's not cancelable? This would only be for iOS ofc.

Tried something like this, it seemed to have worked at first, but it didn't. I can't tell if the issue happens less often with it, of if it makes no difference at all.

_handleOnRequestClose() {
    if (this.props.cancelable) {
      this.close();
    }
    else if(IS_IOS){
      this.close();
      setTimeout(()=> {
        this.setState({visible: true});
      }, 10);
    }
  }

Will do more testing later.

cristianoccazinsp commented 5 years ago

Update: Nope, it's not this neither... Sigh.

vedev9 commented 5 years ago

Same problem here

samithaf commented 5 years ago

Isn't this cause by classic "view is not in the window hierarchy" ios error? If you open up the xcode you can see the warning;)

tattivitorino commented 5 years ago

Well, I'm quite sure it must not be the best way but my workaround for now is wrapping the showAlert in a setTimeout of 500 so it gives the right amount of time to close the spinner in order to open the alert... if any of you find a more "right" way to fix it we all appreciate.. cheers

samithaf commented 5 years ago

Yesterday I tried out wrapping up MBProgressHUD (https://github.com/jdg/MBProgressHUD) with a react native bridge so that I can use within react native view. So far seems working fine with Alert component without colliding. I will post a gist or sample code once I have tested with more devices.

cristianoccazinsp commented 5 years ago

Interesting. Could that be used with this library such that IOS uses that one, while android keeps the current behaviour?

El jue., 6 de junio de 2019 19:14, Samitha Fernando < notifications@github.com> escribió:

Yesterday I tried out wrapping up MBProgressHUD ( https://github.com/jdg/MBProgressHUD) with a react native bridge so that I can use within react native view. So far seems working fine with Alert component without colliding. I will post a gist or sample code once I have tested with more devices.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/joinspontaneous/react-native-loading-spinner-overlay/issues/82?email_source=notifications&email_token=ALU263HJC3BNDL2SNP2RBTDPZGD6DA5CNFSM4HNNFQD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXEKCWA#issuecomment-499687768, or mute the thread https://github.com/notifications/unsubscribe-auth/ALU263ENJK6XXJSZUBEU6MLPZGD6DANCNFSM4HNNFQDQ .

samithaf commented 5 years ago

Technically you can or you can use MBProgressHUD version for Android too, which is awesome. Those modules are pretty popular!. Here is a sample implementation and works perfectly well with Alerts and also look and feel is super neat.

Here is a sample implementation. I will create a node module soon...

https://gist.github.com/samithaf/83efbc7b6d86448d0e4e426c68b29985

phongle2512 commented 5 years ago

Got the same issue, have to do the workaround as tattivitorino's comment. Hope this will be fixed soon

cereme commented 5 years ago

Same issue here. But it only stucks on iOS.

Well, I'm quite sure it must not be the best way but my workaround for now is wrapping the showAlert in a setTimeout of 500 so it gives the right amount of time to close the spinner in order to open the alert... if any of you find a more "right" way to fix it we all appreciate.. cheers

And this works well. Thanks @tattivitorino, Hope some fix to this issue.

hosembafer commented 5 years ago

Same issue when using with react-native-root-toast

niftylettuce commented 4 years ago

RN is super buggy and I stopped using it, something native as you mentioned (MBProgressHUD) would probably do the trick