whitespectre / rn-modal-presenter

A simple to use library to present modals on top of the app using Root Siblings.
MIT License
47 stars 1 forks source link

{children} not behaving as expected #7

Open schonarth opened 1 year ago

schonarth commented 1 year ago

I implemented a custom modal pretty close to your example. The relevant snippet looks like this:

  private showCustomModal = (
    title: string,
    buttons: CustomModalButton[] = [],
  ) => {
    const { children } = this.props
    const ModalContent = (props: CustomModalProps) => {
      return (
        <BaseCompactPopupTemplate {...props} onDismiss={this.onDismiss}>
          {children}
        </BaseCompactPopupTemplate>
      )
    }
    return showModal(ModalContent, {
      title,
      buttons,
    } as CustomModalProps)
  }

Notice that instead of my BaseCompactPopupTemplate component rendering the body directly, it acts as the UI around the modal content, which is provided by each other component that uses it as children. This works as expected when wrapped in the regular React Native Modal component.

While the children components do render properly in the modal, they do not behave as expected. For instance, we have a custom field wrapper that adds label, basic validations, etc. Not only is my input not being properly validated, it's being erased as I type, as though I had not initialized the field.

It seems like while the modal is rendering the content passed as children, it's not keeping the context. None of the event handlers I set up there are being fired.

Am I missing something, or is there another way to pass on-the-fly content to the modal?

Thanks

lurui1029 commented 1 year ago

Hi @schonarth , thanks for using the library!

Issue 1: Notice that instead of my BaseCompactPopupTemplate component rendering the body directly, it acts as the UI around the modal content, which is provided by each other component that uses it as children.

We can help more if you could share more specific code or info about how your BaseCompactPopupTemplate component is constructed, but here's the rule for rendering modal content:

  1. Your entire modal component, BaseCompactPopupTemplate in this case, will be rendered on the center of the full screen overlay which is provided by the library when you call showModal function.
  2. You're responsible for rendering content inside your modal component in a layout that you expect. This means you should set all the proper stylings to your BaseCompactPopupTemplate component and all the children components.

Issue 2: we have a custom field wrapper that adds label, basic validations, etc. Not only is my input not being properly validated, it's being erased as I type, as though I had not initialized the field.

Again, we'd be happy to provide help if you could share more on this. Here's a few things I'd recommend to check:

  1. Could you set up a simpler modal component to render the custom file wrapper you have, without passing it as children, to see if your components work in the modal?
  2. Components on the library's modals can access other React Contexts, but make sure your other context Providers have higher level than the library's ModalPresenterParent. For example, in the following code setup, modals will be able to access ContextA but can't access ContextB:
    const App = () => {
    return (
    ...other nodes
    <ContextAProvider>
      <ModalPresenterParent>
        <ContextBProvider>
        ...more nodes
        </ContextBProvider>
      </ModalPresenterParent>
    </ContextAProvider>
    ...other nodes
    );
    };

    But in case you need to access ContextB, you can simply insert another ModalPresenterParent below ContextB:

    const App = () => {
    return (
    ...other nodes
    <ContextAProvider>
      <ModalPresenterParent>
        <ContextBProvider>
          <ModalPresenterParent>
            ...more nodes
          </ModalPresenterParent>
        </ContextBProvider>
      </ModalPresenterParent>
    </ContextAProvider>
    ...other nodes
    );
    };
schonarth commented 1 year ago

Hi @lurui1029, thank you for your quick reply!

It is difficult to share much of the other components since they're part of a proprietary system I'm not allowed to disclose. Besides, the field wrapper is a complex beast that wouldn't add much here.

Suffice it to say I got it to render as expected, even with the custom looks I needed (e.g. stick the modal to the bottom of the screen). Only the callbacks don't seem to be getting triggered in my {children}'s components.

I'll try and put together an equivalent setup in a solo project and either reproduce the issue there, or find my mistakes in the process, and share it here.

Thanks!

schonarth commented 1 year ago

Turns out when I did it in the smaller scale it worked as expected, meaning I must have gotten something out of place.

Here is the code if you care to take a look.

I tried to reproduce a simpler version of my real-world app: modals open a field wrapper with some specific behaviors. When the field blurs (or the modal closes) the app state must be updated. There's a copy of the field wrapper on the main app body for reference and in each modal for comparison.

schonarth commented 1 year ago

I have 2 pieces of feedback.

  1. I can't install this library, let alone load it once I have, with rn-modal-presenter alone. The whole @whitespectre/rn-modal-presenter path is needed at all times; this may confuse people on their first contact and I suggest you update your instructions 😉
  2. Does the library support class-based components?

As I was building the sample project (above) I just used functional components (and JS) for simplicity, though all components on my employer are class-based. I know functional components are all the rage and all, and I questioned it when I joined the team, but standards are standards, and they aren't changing the whole codebase now.

I realized something was off when my equivalent to CustomAlert wouldn't even parse TypeScript properly, and seemed to work once I rewrote it as a function. I can argue to have that as a function, alright. But then other components using it or being passed to it (like the FieldWrapper, for instance, and more) still aren't being properly initialized.

At this point I wonder if they would need to be converted to functions too, which is not going to happen I know.

Which brings us back to point no. 2 above. If the library only supports function-based components, I can't use it -- and I strongly recommend you make it clear so that other potential adopters know it up-front. Even functional components being the standard nowadays, there are codebases that aren't moving away from classes due to lots of perfectly working, if unfashionable code, to say the least.

Thanks

lurui1029 commented 1 year ago

Hi @schonarth,

  1. Good catch. We will update it to the full path @whitespectre/rn-modal-presenter in README. Sorry about this confusion, and thank you!
  2. The library does support class based components, but you're right that currently Typescript will complain if class components are provided. We will discuss about this and release a version to support class component with Typescript when we can. For now, I'd recommend this workaround:

Assuming you have a class based component called ModalA here, you don't need to rewrite it to a function component, instead, you can create a temporary function component (Wrapper) right before you call showModal to suppress Typescript errors :

type ModalAProps = {
  width: number;
  height: number;
};

const showModalA = (props: ModalAProps) => {
  const Wrapper = (props: ModalAProps) => {
    return <ModalA {...props} />;
  };
  showModal(Wrapper, props);
};

class ModalA extends Component<ModalAProps> {
  render(): React.ReactNode {
    return <View style={{ width: this.props.width, height: this.props.height, backgroundColor: 'red' }} />;
  }
}

Edit: despite of the Typescript error, class components should work as same as function components do.