Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PROPOSAL: JS version of react-native-modal #145

Closed
mmazzarolo opened this issue Apr 18, 2018 · 62 comments
Closed

PROPOSAL: JS version of react-native-modal #145

mmazzarolo opened this issue Apr 18, 2018 · 62 comments

Comments

@mmazzarolo
Copy link
Member

Hello everyone!
I wanted to open this "thread" to get some feedback on a new implementation of react-native-modal I'm building.

As you may already know, the current version of react-native-modal is based on react-native's original modal, which, unfortunately, has many different issues that are difficult to solve since it would involve native code.

To completely solve (almost all of) these issues I'm building a 100% Javascript version of react-native-modal that doesn't use the react-native's original modal.
The main issue of using a total JS solution is that you should place it on the root of you app to make it work (because otherwise the modal won't be able to overlay the entire screen), but I'm testing a portal-like solution that should overcome this block (and it seems to be working!).

Are you interested in it?
Do you think it would solve any of your issues with the current modal?
Do you think it would be useful releasing this WIP in a separate beta branch?

Any feedback is welcome!

@ChristianTucker
Copy link

ChristianTucker commented Apr 19, 2018

I think this is great, I usually use a JS based modal and just recently started using react-native-modal in this project and as discussed in #144 I may just go ahead and go back to this. I just wanted to use react-native-modal as it came standard with the PanResponders and the react-native-animated library already built in.

If you need any help with this rewrite, let me know I'll be happy to contribute.

We do the following in our applications (using our standard modals and with react-native-modal)

<Provider store={redux.store}>
    <View note="application container" style={{ flex: 1 }}>
        {component}
        <Modal />
    </View>
</Provider>

Our <Modal /> component is built to render the modal using an absolute positioned element and interacts with a Redux reducer specifically designed for the modal. All of our reducers have the following actions (SET_X_STATE, X_STATE, and RESET_X_STATE). SET overwrites, RESET sets the state to initial values and X_STATE uses deep-merge.

So we wrapped the react-native-modal props like so:

import React from 'react'
import { View } from 'react-native'
import { createReducer } from 'powerline/utils'

export default createReducer('modal', {
    animationIn: 'slideInUp',
    animationInTiming: 300,
    animationOut: 'slideOutDown',
    animationOutTiming: 300,
    avoidKeyboard: false,
    backdropStyle: null,
    backdropColor: 'black',
    backdropOpacity: 0.7,
    backdropTransitionInTiming: 300,
    backdropTransitionOutTiming: 300,
    children: <View />,
    isVisbile: false,
    onClose: () => {},
    onBackButtonPress: () => {
        const { store } = require('powerline/instances').redux
        store.getState().modal.onClose()
        store.dispatch({
            type: 'MODAL_STATE',
            payload: { isVisible: false }
        })
    },
    onBackdropPress: () => {
        const { store } = require('powerline/instances').redux
        store.getState().modal.onClose()
        store.dispatch({
            type: 'MODAL_STATE',
            payload: { isVisible: false }
        })
    },
    onModalHide: () => {
        require('powerline/instances').redux.store.dispatch({
            type: 'RESET_MODAL_STATE'
        })
    },
    onModalShow: () => {},
    onSwipe: null,
    swipeThreshhod: 100,
    swipeDirection: null,
    useNativeDriver: false,
    hideModalContentWhileAnimating: false,
    style: {
        margin: 0,
        width: '100%',
        height: '100%'
    }
})

We added an onClose that we use instead of onModalHide that's called when the modal is closed, that way we can execute our own close logic without overwriting the onModalHide which automatically resets the modal back to the initial state after it's done being transitioned off the screen.

This way we can use

this.props.dispatch({
    type: 'MODAL_STATE',
    payload: { 
        isVisible: true,
        children: <MyModalComponent /> 
    }
});

to show a modal and

this.props.dispatch({
    type: 'MODAL_STATE',
    payload: { isVisible: false }
});

to close a modal, without having to worry about changing to the props from the previous modal affecting the next one.

Just some ideas if you want to integrate some sort of static Modal API.

I think that adding a <Modal /> component on every component that you need a fullscreen modal is a bad implementation, and that just having a at the root rendering ontop of everything else is much cleaner.

  • it allows you to be able to interact with the modal anywhere. (I.e, external business logic that's separated from components).

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Apr 19, 2018

Thanks for the feedback and for starting the discussion!

I think that adding a component on every component that you need a fullscreen modal is a bad implementation, and that just having a at the root rendering ontop of everything else is much cleaner.
I understand your position (I used it too on some projects) but I disagree on using a single root modal because:

  • If you're using a single modal in your entire app (dialog-like for example) it's perfectly fine... but when you need more than one it gets a bit more complex
  • Controlling it is harder (and will probably force you to use state managements libraries/context)
  • Customizing it would be a pain when using more than one modal

it allows you to be able to interact with the modal anywhere. (I.e, external business logic that's separated from components).

You can always wrap it easily in a component and connect it to Redux if you really need (no root modal needed)


That said, my proposed solution it's the following.

  1. Place a provider/portal at the root of the app (I'll call it ModalPortalOut in this example):
class Root extends Component {
  render() {
    <View>
      <App />
      <ModalPortalOut />
    </View>
  }
}
  1. Whenever you need to use the JS modal, just wrap it in ModalPortalIn and it will be rendered* inside ModalPortalOut (at the root of your app then).
class RandomComponent extends Component {
  render() {
    <View>
      <App />
      <ModalPortalIn>
        <JSModal isVisible={true}>
          {...modal content}
        </JSModal>
      </ModalPortalIn>
    </View>
  }
}

*it will be rendered: this is the hard part, since portals are still not officially implemented in react-native

@theohdv
Copy link
Contributor

theohdv commented Apr 19, 2018

In your example @mmazzarolo, how you'll be able to dynamically update the modal props if you are in a deeper component of your app?

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Apr 19, 2018

@theohdv In the same way you do it in the current react-native-modal.

The issue I'm trying to solve by moving react-native-modal to a fully JS version is not how you send props to it (you can already connect it to Redux/Mobx easily if you want) but it's the pack of bugs related to react-native modal, like the Android status bar bug and the modal + dialog at the same time bugs.

The only difference (I'd like to have) in the new react-native-modal api would be adding the portal/provider in the root of you app and nothing else.

@ChristianTucker
Copy link

ChristianTucker commented Apr 19, 2018

@mmazzarolo That's also an alright concept, but I don't really feel like it's the "right" way. Granted, this is all a matter of opinion, but something about being able to offload showing modals to my business logic (which is decoupled from my components) seems really nice. For example, let's say I have some sort of relatively complex screen that has a few different error modals (with different designs), a success modal, and a login modal.

Doing it the way you proposed would require you to have a bunch of conditional rendering, where-as just rendering the children through a state management system and handling it through an API allows you to just easily pick what you need.

modal.show(<LoginModal />, { ...options });
modal.hide()

I can't really think of any scenarios where you'd be rendering multiple modals at the same time purposely.

This allows for less render/class pollution, imo. I really wouldn't want to have several different { modalType == '...' && this.renderModalType() } calls, (or similar)

We also share our business logic between mobile and web, so for us this will also open our LoginModal on our React web-application as well. Instead of requiring us to write duplicate code. (Albeit the modal implementation)

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Apr 19, 2018

modal.show(<LoginModal />, { ...options });
modal.hide()

Ah! So this is the way you show and hide the modal.
I understand you point of view, but I don't like the approach since it means mixing the React pattern with a more procedural one (and that's the reason in react-native-modal we don't officially support the "ref" actions).
Also, if you like showing the modal from anywhere in your code, there's nothing stopping you from connecting the current react-native-modal to Redux and calling showModal from anywhere.

this is all a matter of opinion
Yes, indeed 👍

Thanks for sharing your pattern!

@ChristianTucker
Copy link

Would be interested to see how you inject the <ModalPortalIn /> into the <ModalPortalOut /> component, so keep me posted on when you push this.

@hellogerard
Copy link

We ran into the same limitations with React Native's built-in Modal component that you did. I don't know if you've seen this one, but we've been using https://github.com/magicismight/react-native-root-modal instead. Just an alternative implementation, FYI.

I prefer being able to drop my modal where ever it's opened from. I don't want to go into the root of my app and change code, just open a modal. Exception to that: we have features that open a completely new area of the app, with additional screens, all within a modal. For those, we just use a vertical transition on the stack navigator at the root of our app since there are no fancy interactions and it is always fullscreen. I guess the difference is that sometimes you need a "quick" modal that is pertinent to the screen it is on, may not be full-height, is animatable/swipeable, and should live with its parent. Sometimes you just need to navigate to an area of your app and cover the full screen.

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Jul 6, 2018

Hey @hellogerard !
Yes, I'm aware of that solution too, it uses react-native-root-sibling under the hood.
react-native-root-sibling injects a wrapper at the root of the app by calling AppRegistry. setWrapperComponentProvider and honestly it's still not clear to me if this is a 100% safe solution to adopt (last time I used it I had a few issues with external providers)... 🤔

@rayandrew
Copy link

Hello, I created ported version of React Native Modal using react native web component.

you can check it on: https://github.com/rayandrews/react-native-web-modal

There are 2 packages, basic modal (replacement of React Native's modal) and enhanced modal (based on React Native Modal by React Native Community)

@mmazzarolo
Copy link
Member Author

@RayAndrews this is great!
Let me know if we can help/improve it!

@rayandrew
Copy link

rayandrew commented Jul 10, 2018

@mmazzarolo, yes you can improve it!

I'm currently using ReactDOM portal so React version must be over 16.x.x.

React Native Modal by React Native Community is work so good, I already used it in productions.

The one that is not complete is ported version of React Native's modal. Not all props are supported and I hope it can be merged into React Native Web soon (if @necolas agree with the code, of course)

Beware: NOT SSR Ready
SSR READY!

@hellogerard
Copy link

hellogerard commented Aug 9, 2018

Here is an implementation of native (ObjC, Java) portals to get around the relative-to-parent problem. One big value-add with this one is that since it's on the native side, it uses the same instance of the modal child, instead of creating a new one. So, for example, if you're playing a video inline on a screen, you can open it in a modal without the video restarting.

https://github.com/mfrachet/rn-reparentable

@mmazzarolo
Copy link
Member Author

Let's keep track of this

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Sep 16, 2018

I created a quick POC of the JS Modal using React Native Paper implementations of Portals.
You can try it from the portal branch.
The setup of this version of the modal is slightly different from the standard version.

  1. Wrap your entire app in ModalHost:
import { ModalHost } from 'react-native-modal';

...

<ModalHost>
  <App />
</ModalHost>
  1. Use the <Modal /> component anywhere in your app like just, like your used to do, but import it this way:
import { Modal } from 'react-native-modal':

All the react-native-modal props seem to work correctly.
Also, check the example folder for an updated example.

@Titozzz
Copy link
Contributor

Titozzz commented Sep 16, 2018

Quick update, I've been playing with @mmazzarolo code and official portals 🔥
https://twitter.com/titozzz/status/1041393720288002059

Fork
Demo

Looks like multiple modal are working fine

@mmazzarolo
Copy link
Member Author

🎉 🎉 🎉 🎉 🎉

@Titozzz
Copy link
Contributor

Titozzz commented Oct 7, 2018

@mmazzarolo Let's get this going, what do you think ? What can I do to help

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Oct 8, 2018

Hey @Titozzz , yeah sorry I've been super busy lately with work/tutorials/other stuff 😞

I'm still not 100% sure on how to handle update since it will be a highly breaking change.
Should we just create a next/beta branch and allow people to test it a bit before releasing it officially? What do you think?
Also, since it looks like RN is slowly migrating its core component to the react-native-community organization it would be great hearing out what are the plans about the original native modal.

P.S.: If you're willing to help on this, I'd gladly add you as core contributor to the repo.

@Titozzz
Copy link
Contributor

Titozzz commented Oct 8, 2018

I'm part of the guys doing the react-native-community slimmening on the webview, so I can ask in the core slack if there are any plans if you want. I'm trying to get it to work on my main project and will keep you updated

@mmazzarolo
Copy link
Member Author

Yes, please.
Thank you @Titozzz

@Titozzz
Copy link
Contributor

Titozzz commented Oct 8, 2018

Damn I did not successfuly got my demo working on iOS .. Android is fine tho.. I'll keep trying

@mmazzarolo
Copy link
Member Author

Let me know if you need any help.

@Titozzz
Copy link
Contributor

Titozzz commented Oct 9, 2018

Can you install the demo to verify you have the same issue on iOS that I do and android is working ?

@mmazzarolo
Copy link
Member Author

Not working for me.
What error did you get?

screen shot 2018-10-10 at 15 52 19

@Titozzz
Copy link
Contributor

Titozzz commented Oct 10, 2018

Yeah same, only on iOS right ?

@mmazzarolo
Copy link
Member Author

Yes, Android works fine.

@Titozzz
Copy link
Contributor

Titozzz commented Oct 10, 2018

🤔 This is as annoying as it is hard to debug haha 😅

@scottmas
Copy link
Contributor

scottmas commented Apr 2, 2019

It seems to me that lots of this back and forth could be mitigated if we simply exposed an API to let the user customize which Modal implementation they actually want to use. E.g.

import React from 'react';
import { Text, Modal as RNModal } from 'react-native';
import { createModalComponent } from "react-native-modal";
import MyCustomModal from './libraries/my-custom-modal'
// const Modal = createModalComponent(RNModal) //The default implementation
const Modal = createModalComponent(MyCustomModal);

export default App extends React.Component{
  render(){
    return <Modal isVisible={true}><Text>Hello world!</Text></Modal>
  }
}

By providing an API like this, the user can decide if they want to do a portal based solution, use a setWrapperComponentProvider type solution, or so on. @mmazzarolo Would you be open to a PR implementing this API? In the near term, this solves some problems I'm facing that I want to use the base Modal provided by react-native-root-modal.

P.S. To answer your question, react-native-root-modal exposes both a declarative and procedural API. IMO, the procedural API is superior for many use cases, such as a confirm modal, where the dev doesn't want to manage the modal's lifecycle, he just wants the result of the modal interaction.

@scottmas
Copy link
Contributor

scottmas commented Apr 2, 2019

Just an update: I actually took a stab at implementing a PR to implement the above proposed pluggable Modal API. It got hairy quickly, at least in conjunction with react-native-root-modal. There's a fair bit of implicit behavior in the vanilla react native modal that this library is relying on - for instance I had to implement shouldComponentUpdate to make the custom component work. Also, some weirdness I think with contentRef. Without diving deeper into it, it's hard to know the precise implicit behavior this library is relying on.

@mmazzarolo
Copy link
Member Author

@scottmas thanks for continuing the discussion and giving a try with a PR 🙌

for instance I had to implement shouldComponentUpdate to make the custom component work

You can get an example of a "working" JS solution from @Titozzz 's fork or from the portal branch.

I think in a previous post we talked about abstracting the modal "container" so that we can use both a JS and native behaviour... It makes sense, but as you might have noticed while playing around with the PR, it would increase the complexity of maintaining both approaches, which brings back the question: why should we support the "native modal" approach if we have a fully functional JS one? 🤔 What are the benefit of the native implementation over the the JS one (accessibility?)

@neonsec
Copy link

neonsec commented Apr 6, 2019

I am using @Titozzz 's fork of react native modal. I was looking to make my android app full screen without having the bottom navigation bars, but backdrop did not cover up the full screen.
Is there any workaround for this?
modal-fullscreen

@neonsec
Copy link

neonsec commented Apr 6, 2019

Update: To fix this, I had to use Dimensions.get('screen') instead of Dimensions.get('window') for the backdrop to fill the entire screen.

@mmazzarolo
Copy link
Member Author

Another pure JS cool solution! https://github.com/colorfy-software/react-native-modalfy

@RichardLindhout
Copy link
Contributor

It even works on the web now ;)

@RichardLindhout
Copy link
Contributor

The portal branch!

@BrendonSled
Copy link

Heres an alpha version of a version I made:
https://github.com/BrendonSled/react-native-remodal

it uses react-native-reanimated to keep performance top notch while supporting things like multiple dialogs and custom animations

@mmazzarolo
Copy link
Member Author

@BrendonSled is it a 1:1 rework of react-native-modal? (can't check right now)
If you're interested we're still looking for new maintainers/collaborators for the current modal rework 👍

@BrendonSled
Copy link

BrendonSled commented Sep 21, 2019

@BrendonSled is it a 1:1 rework of react-native-modal? (can't check right now)
If you're interested we're still looking for new maintainers/collaborators for the current modal rework 👍

Not quite, there's a pretty major setback too with react native not having react's portal integration. Setting views through the context (like what react-native-portals does and this implementation) anytime that views tree updates the entire tree rerenders. This causes things like text input in the views to significantly delay.

If anyone has a solution there I'm all ears.

React native does have a solution but it currently is broken on iOS

import { createPortal } from "react-native/Libraries/Renderer/shims/ReactNative";

@mmazzarolo
Copy link
Member Author

@BrendonSled gotcha, I wasn't aware the portal implementation was still broken on iOS

@joaovpmamede
Copy link

I wish that callstack would make this portal implementation as a separate package. Could help in your situation @BrendonSled

@BrendonSled
Copy link

I wish that callstack would make this portal implementation as a separate package. Could help in your situation @BrendonSled

This portal implementation is the same as my implementation. They both rely on context updating state. The issue there is anytime your component changes (including its children) the entire component is rerendered. For the most part this is fine, but when you have things like text inputs they will lose their focus because of the rerender which makes typing in a dialog impossible.

@emac3 emac3 mentioned this issue Mar 19, 2020
@mmazzarolo mmazzarolo pinned this issue Aug 22, 2021
@intergalacticspacehighway
Copy link

intergalacticspacehighway commented Aug 25, 2021

Got the RN portal working here https://github.com/intergalacticspacehighway/rn-portal-test
Didn't have to do much. This assertion was causing this issue. It appears that the assertion only allows root instance tags. I made it work by commenting it but it works if you pass the root instance's tag e.g. the one which we get as soon as the app loads (root tag id).

@RichardLindhout
Copy link
Contributor

@intergalacticspacehighway
Copy link

intergalacticspacehighway commented Aug 25, 2021

@RichardLindhout This solution is great. The only downside is that intermediate React context would need some workaround as it mounts the children in Host. This won't be an issue in RN's createPortal API. Also, the state update happens with useEffect which might add slight delay.

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Aug 25, 2021

I assume we resurrected the discussion because of the blog post?
Thanks @intergalacticspacehighway and @RichardLindhout .
@RichardLindhout are you familiar with/using RN-portal? Any feedback/known issue?

@nandorojo
Copy link

What about: https://github.com/gorhom/react-native-portal#readme

While this library is nice, it isn't a true React portal. Since it mounts elements outside of their position in the React tree, they lose any context they might be wrapped in. This isn't acceptable for many use cases.

The RN portal usage would be a good solution, so long as there aren't any actual modals rendered on top of the portal.

@mmazzarolo
Copy link
Member Author

Is anyone familiar with the portal implementation used in react-native-paper? https://github.com/callstack/react-native-paper/tree/master/src/components/Portal

@mmazzarolo
Copy link
Member Author

mmazzarolo commented Aug 29, 2021

Got the RN portal working here https://github.com/intergalacticspacehighway/rn-portal-test
Didn't have to do much. This assertion was causing this issue. It appears that the assertion only allows root instance tags. I made it work by commenting it but it works if you pass the root instance's tag e.g. the one which we get as soon as the app loads (root tag id).

Dang it @intergalacticspacehighway , I didn't notice this comment, my bad.
I'll play with this soon. Do we have an existing PR addressing what is being patched?

@react-native-modal react-native-modal locked and limited conversation to collaborators Aug 29, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests