feat: Add overlayAsync implementation

[XionWCFM]

Description

This change implements the openAsync method for overly-kit's promise support discussed in #47.

Related Issue: Fixes #47

Changes

• The openAsync method is added to the overlay object of event.ts that is exposed externally

Motivation and Context

Although overlay-kit emphasizes support for promises in its documentation, there was a problem with boilerplate due to the lack of explicit support for promises.

To address this issue, we add openAsync explicit support for promises.

How Has This Been Tested?

I tested through test code that the parameters passed to openAsync's close method return resolved values, and that the close method properly closes the overlay on the screen.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have performed a self-review of my own code.
• My code is commented, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• Any dependent changes have been merged and published in downstream modules.

Further Comments

• In order to adhere to the DRY principle and minimize modifications to existing code, openAsync was implemented in the form of wrapping open. Is this implementation acceptable?

• The close method of openAsync is implemented to require parameters. The reason I implemented it this way is because if an optional parameter is allowed, type assertion through as is required internally, which makes it type-safe, or there is the inconvenience of the user having to infer undefind even though the confirmation value is actually passed. I also chose a type-safe implementation because we believed that the core of Promise use cases lies in using checked values. I'm curious about your opinion on this part.

• Would it be a good idea to add docs for openAsync in that PR? If I had to add it, I would like to know which document it would be best to add it to. I am prepared when it comes to writing documents.

[changeset-bot]

🦋 Changeset detected

Latest commit: e91db62

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
overlay-kit	Minor
@overlay-kit/framer-motion	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
overlay-kit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 15, 2024 5:50am

[jgjgill]

I thought it was odd that await wasn't attached here.
I'll share my thoughts as I test it out. If I'm wrong, feel free to tell me. 🙇‍♂️

Test fails when await is appended

I only added await in the code you provided, so the test fails.

await waitFor(() => {
  expect(dialogContentElement).not.toBeInTheDocument();
});

I think the reason the test passed without the await is because the test inside the waitFor works asynchronously, but the test function doesn't wait for it.

So if I don't add await, I think I can write a test like this and it will pass.

waitFor(() => expect(dialogContentElement).toBeInTheDocument());

Why does it fail?

The next question is where the test fails when you add await.
close is not removed from the DOM, so not.toBeInTheDocument() seems appropriate with unmount.

I can't think of a way to test the close behavior right now. sorry. 😂
I'll have to think about that.

[XionWCFM]

You are right. It was my mistake to omit the "await" and the test would return a false positive.

Thank you for catching my mistake

The reason it fails is because "close" does not actually unmount the element, but rather changes the value of isOpen.

Therefore, I modified the code as follows:

      const handleClick = async () => {
        overlay.openAsync<boolean>(({ isOpen, close }) =>
          // focus on isOpen
          isOpen ? <button onClick={() => close(true)}>{dialogContent}</button> : null
        );
      };
...
    await waitFor(() => {
      expect(dialogContentElement).not.toBeInTheDocument();
    });

This code allows the button to be unmounted from the screen based on the value of isOpen.

A commit that resolves the issue has been pushed to this PR.

Thank you for checking carefully. With your help I was able to find this problem

[jungpaeng]

Thank you for your contribution! The code you provided looks good to me.

One concern I have is that we can’t resolve through overlay.close. I plan to create a separate PR to add an option for this.

I am thinking of an API like overlay.close(overlayId, { resolve }). If you have any opinions on this, please feel free to share.

Thanks again!