-
Notifications
You must be signed in to change notification settings - Fork 123
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
Comments
Hi @MarksCode , thanks for the PR #89 , it's a good proposal. However IMO it seems not common enough, why So, my thought is adding a new method Another thing is, if we add this, should we also auto resolve the modal in helpers like @Alexandredc what's your thought since you upped this? :-) |
Why not the Or maybe the // 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 |
@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 Went with |
这个PR实际上可以更大胆一点。
|
上周我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)的默认值如何选择还需要更多的调研做决定。 |
In all my modal components, I either return a response, or if the user closes the modal I do something like this:
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 themodal
instance that did both of these steps in one. 🙌The text was updated successfully, but these errors were encountered: