Skip to content
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

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

Open
MarksCode opened this issue Nov 17, 2022 · 5 comments
Open

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

MarksCode opened this issue Nov 17, 2022 · 5 comments

Comments

@MarksCode
Copy link
Contributor

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
Copy link
Collaborator

supnate commented Nov 20, 2022

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
Copy link
Contributor

alexandredev3 commented Dec 15, 2022

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
Copy link

adrhumphreys commented Jan 18, 2023

@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
Copy link

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

  • 一个好的办法是
  1. hide 支持传参。默认resolve。
  2. 可以的话,不需要暴露 resolveHide、 remove 等方法。 在NiceModal.open(Component,props,config/新增配置/) config:{ remove : boolean ; once:boolean;等等}即可。

@raotaohub
Copy link

raotaohub commented May 28, 2023

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

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

  • 我尝试过新增 ConfigProvider 暴露给用户允许全局配置某些默认行为(我称它为hide的默认行为),然后在新增 Modal.open(C,{},config) 来控制单个Modal 的默认行为。

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants