toss / overlay-kit

The simplest and most intuitive way to manage overlays in React.
https://overlay-kit.slash.page
MIT License
295 stars 30 forks source link

Suggestion: Stronger support for promise use cases #47

Closed XionWCFM closed 4 months ago

XionWCFM commented 4 months ago

Summery

It's not a problem, but I cautiously suggest that it would be better if overlay-kit had this function.

Could you please consider adding explicit support for Promise to the overlay kit?

Feature Request

I'm opening an issue because there's a feature I'd like to suggest.

overlay-kit documentation provides some nice examples of using overlay-kit with Promise.

It works well, but as you can see in the documentation examples, creating Promise, creating callbacks, and opening overlays feels like a lot of boilerplate

I think it would be good to have a feature that reduces boilerplate and easily supports Promise use cases.

The current usage is as follows:

const result = await new Promise<boolean>(resolve => {
  overlay.open(({ isOpen, close }) => {
    return (
      <AlertDialog 
        open={isOpen} 
        title="Are you sure?"
        onConfirm={() => {
          resolve(true);
          close();
        }}
        onCancel={() => {
          resolve(false);
          close();
        }} 
      />
    )
  })
});

The rough API I think of is as follows.

const result = await overlay.promise(({ isOpen,close, resolve}) => (
  <AlertDialog 
    open={isOpen} 
    title="Are you sure?" 
    onConfirm={() => {
      resolve(true);
      close();
    }}/>
    onCancel={() => {
      resolve(false)
      close()
    }}
  ))

The nice thing about this implementation is that logic like creating new Promise is abstracted away, reducing boilerplate.


I tested a simple piece of code that implements this use case and found that it worked great.

If this suggestion looks good, I'd like to work on it.

The experience and mental model of using this library are excellent. Thank you for creating a good library.

Please consider my offer. Bye.

jungpaeng commented 4 months ago

Hello, Thank you for your thoughtful suggestion.

As you mentioned, using overlay-kit with promises can indeed create a lot of boilerplate, which might feel cumbersome. Adding explicit support for this would likely improve productivity significantly.

I think the functionality proposed with overlay.promise is excellent. However, I would like to suggest renaming this new method to overlay.openAsync. This name would maintain consistency with the existing method overlay.open, enhancing the intuitive understanding of its asynchronous nature.

Also, in your implementation of overlay.promise, the callback function includes a resolve function as an argument. I wonder if there are use cases where the resolve and close steps need to be separate. If not, perhaps we could streamline this by allowing something like close({ resolve: true }) or close(true). This could make the intention of “closing and returning a value” more explicit.

Let me know what you think about these suggestions.

XionWCFM commented 4 months ago

The name overlay.promise was inspired by sonner, a toast library. This is because sonner supported asynchronous use cases with the method name toast.promise().

However, compared to overlay.promise, overlay.openAsync is more intuitive as you mentioned and maintains consistency with open. Therefore, it seems good to use openAsync.


Moving on to the topic of separation of the resolve phase and the close phase, when I first thought of that rough API, I thought of that type of API to provide a wider degree of freedom to the user.

(Please note that I am not familiar with the design philosophy of overlay-kit)For special use case, I think it can be solved by applying the methods from the overlay-kit documentation.

So I think overlay.openAsync would be better off focusing on common use cases.

Therefore, among the methods you suggested, the close(true) type API seems better to me. I'm curious to know your opinion.


I'm interested in working on providing support for asynchronous use cases for this overlay. Could I possibly get a chance to work on implementing this overlay.openAsync?

jungpaeng commented 4 months ago

I'm interested in working on providing support for asynchronous use cases for this overlay. Could I possibly get a chance to work on implementing this overlay.openAsync?

Certainly! It would be a great help if you could assist with the task.

If you need any help during the process, please feel free to share your thoughts at any time.

XionWCFM commented 4 months ago

wow , let me work on this issue.

thank you!