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

Remove confirmModalPromise #10025

Closed
cjcenizal opened this issue Jan 23, 2017 · 0 comments
Closed

Remove confirmModalPromise #10025

cjcenizal opened this issue Jan 23, 2017 · 0 comments
Labels
enhancement New value added to drive a business result stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@cjcenizal
Copy link
Contributor

Per #9984 (comment) discussion, let's remove confirmModalPromise and just use confirmModal instead.

It seems a bit confusing to have two ways of doing the same thing, only with vey slightly different interfaces. Are there use cases where it's better to use one over the other?

Unless there's a compelling reason to use confirmModalPromise over confirmModal, then I think we're adding unnecessary complexity by keeping both. If there are clear use-cases where confirmModalPromise should be used over confirmModal, then I think we need to change the name to reflect that use-case (instead of simply reflecting an implementation detail).

Since confirmModalPromise only being used in two places, I think we should just create and return the promises locally at each call site, and just use confirmModal instead.

We can also dig deeper and see if the promises are even necessary.

@cjcenizal cjcenizal added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:enhancement labels Jan 23, 2017
@chrisronline chrisronline mentioned this issue Mar 12, 2018
47 tasks
@joshdover joshdover added the stale Used to mark issues that were closed for being stale label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants