eBay / nice-modal-react

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

[Request] Add a modal.removeResolve(false) helper function #88

Open MarksCode opened 1 year ago

MarksCode commented 1 year ago

In all my modal components, I either return a response, or if the user closes the modal I do something like this:

<Modal
    onClose={() => {
       modal.resolve(false);
       modal.remove();
    }}
>

However, this is not ideal since I'm passing a new onClose function instance on each render. It would be great if there was a utility function on the modal instance that did both of these steps in one. 🙌

supnate commented 1 year ago

Hi @MarksCode , thanks for the PR #89 , it's a good proposal. However IMO it seems not common enough, why false means user cancelled the modal without doing anything? undefined would be better? I understand in many cases we want to resolve a modal before remove. I was struggling with if it should auto resolve the modal when call modal.hide. But finally I kept the flexibility to leave them separately.

So, my thought is adding a new method modal.resolveAndRemove(value) which accepts resolved value. If no arg provided, resolve to undefined.

Another thing is, if we add this, should we also auto resolve the modal in helpers like muiModal, antdModal, etc.

@Alexandredc what's your thought since you upped this? :-)

alexandredev3 commented 1 year ago

Why not the resolve method, when called, resolves a promise and hides the modal automatically in the same call? I can't think of any use case where someone would call the resolve method and not have to hide the modal right after.

Or maybe the resolve could take something like a autohide as a second parameter.

// Resolves to `undefined` and hides the modal automatically.
modal.resolve(undefined, true);

// Resolves to `undefined` and keeps the modal visible
modal.resolve(undefined, false);

But honestly, I think there's no reason not to hide the modal automatically when calling the resolve method.

adrhumphreys commented 1 year ago

@supnate I made a PR for this one with what you had in mind (#99) with the idea of it being an additional function so that we don't break backwards compatibility (e.g. making resolve auto hide would be, even though I would prefer it)

Went with resolveAndHide although if you can think of a more elegant (and shorter) method name that would be nice as it does feel lengthy

raotaohub commented 1 year ago

这个PR实际上可以更大胆一点。

raotaohub commented 1 year ago

上周我fork了项目,并尝试为其新增返回值类型推导,新增resolveAndHide方法等。通过深入阅读源码,我领会到 nice-modal-react 设计的巧妙,这用起来就像react一样的灵活,但正是如此,我难以为实现供优雅的返回值类型推导方案,另外新增方法在我看来的确会提高心智负担,为此我放弃了提交PR。

顺便说一下,我创建了另一个项目 ez-modal-react 来解决上述2个问题。

权衡之后,我选择使用在open方法中新增参数允许用户配置单个Modal的默认行为。对于复杂业务而言,用户可以选择在config里配置,既不默认resolve也不默认remove的场景,我认为是少数的。对于一般业务而言,默认resolve,默认remove是绝大多数的。各大UI库的弹窗组件长久以来都是这么做的。

我认为您也可以采取这样的方案,这样依旧保证了nice-modal-react的灵活性,同时也简化大部分场景的操作。

考虑到 nice-modal-react用户基数交大,这个(config | option)的默认值如何选择还需要更多的调研做决定。