eBay / nice-modal-react

A modal state manager for React.
https://ebay.github.io/nice-modal-react
MIT License
1.96k stars 109 forks source link

Support multiple nested context #119

Closed wjaykim closed 12 months ago

wjaykim commented 12 months ago

Hi, thank you for providing this awesome library. I'm using nice-modal to display modal in react-native.

In some cases, I need to nest multiple NiceModal.Provider to support multiple screens (It's common to nest contexts when using react-navigation: navigation library of react-native).

In the case, since dispatch function is updated on every render phase, nice modal state update doesn't work properly.

With my change, dispatch function is updated only when component is mounted. If there were already existing dispatch assigned, it saves the previous value and restore it when component is going to be unmouted. By doing this, we can use child-most NiceModalContext's dispatch every time.

Let me know if this change is anti-pattern or breaking change for your case!

supnate commented 12 months ago

Hi @wjaykim , thanks for the PR and reasonable use cases! The implementation looks good to me.

I have one question, if a screen is hidden, does it always mean it's unmounted? Does it depends on implementation or some configuration? I'm not familiar with react-navigation, while a page navigated out, is there any case the page is just hidden but not destroyed? Or is there any other navigation libs have different implementation?

wjaykim commented 12 months ago

My intended behavior is to remove all opened modals when screen is unmounted. While it depends on navigator's option, in stack navigator, when new screen(B) is pushed to stack and previous screen(A) goes hidden, previous screen(A) is not going to be unmounted. When user goes back to previous screen(A), current screen(B) is unmounted and removed.

FYI, You can read more about screen(page) lifecycle of react navigation in this documentation: https://reactnavigation.org/docs/navigation-lifecycle#example-scenario

supnate commented 12 months ago

hmm, I understand that. So this is a react-navigation implementation, it never caches a screen. My concern is if a screen is cacheable (maybe other similar nav libs do), then the dispatch maybe not the correct one.

That is, if multiple modal provider on dom tree at same time, it's not predictable if the current one is expected.

So, one option is clearly define which modals belong to which screen declaratively:

<ScreenPageRoot>
  <MyNiceModal1 id="my-modal-1" />
  <MyNiceModal2 id="my-modal-2" />
</ScreenPageRoot>

Then you can use below API to manage the modals:

const modal = useModal('my-modal-1');
// or
NiceModal.show('my-modal-1', props);

This option allows to specify where the modal should be rendered while it's visibility state is still managed at root level.

What do you think?

wjaykim commented 12 months ago

I see. It seems I can(need to) use declarative way. I'm closing this PR!

supnate commented 12 months ago

Anyway, your PR is very inspiring, it isolates modals and state in a separated scope. We will re-consider the idea by avoiding global dispatch method. Thanks again!