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

Added events to dialogue onClose prop method #2944

Conversation

DeveloperTheExplorer
Copy link

Motivation

To provide more information to the onClose function to allow developers to add their own custom logic based on the event fired. Currently, there seems to be no great way to do this. An example scenario where this is necessary is working with react-toastify.

While wrapping the <ToastContainer .../> with <Portal></Portal> (as suggested here) tags allows the toasts to show up on top as expected, anytime they are clicked, the dialog is dismissed, which can lead to unexpected behaviour.

Here is an older discussion on this, note that this solution is not ideal as per MDN's recommendation to avoid using window.event.

Changes Made

New type is declared and exported:

export type DialogEventType = ReactMouseEvent | PointerEvent | FocusEvent | TouchEvent | KeyboardEvent

And it is used as an optional parameter for onClose

export type DialogProps<TTag extends ElementType = typeof DEFAULT_DIALOG_TAG> = Props<
  TTag,
  DialogRenderPropArg,
  DialogPropsWeControl,
  PropsForFeatures<typeof DialogRenderFeatures> & {
    open?: boolean
    onClose(value: boolean, event?: DialogEventType): void // <--- Added here
    initialFocus?: MutableRefObject<HTMLElement | null>
    role?: 'dialog' | 'alertdialog'
    autoFocus?: boolean
    __demoMode?: boolean
  }
>

Copy link

vercel bot commented Jan 25, 2024

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

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2024 0:45am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2024 0:45am

@DeveloperTheExplorer
Copy link
Author

@RobinMalfait Sorry to ping you specifically, but it would be nice if I can get this PR through or get some feedback on this. Thanks.

@parkerholladay
Copy link

I was actually just looking at doing something similar, but for a more limited scope. i.e. Having a close reason rather than a boolean (which is always false) would be very useful for scenarios when we want to disable Esc or outsideClick handling. This proposed solution would enable the same type of logic while also fixing other scenarios when things do appear on top of/outside of the dialog that get clicked.

@DeveloperTheExplorer
Copy link
Author

@parkerholladay I agree, it would be nice if the good people maintaining this project provided some feedback or approved this PR. It has been sitting here for a while now...

@RobinMalfait
Copy link
Member

Hey!

This has come up a few times and we do want to provide some information to the users, so I 100% get why you made the PR which I appreciate! However, we want to tackle this in a slightly different way and do this for most components so that the APIs are consistent.

I will open a PR where we do this for most components and make sure to link to the various PRs that wanted to introduce this as well so that they are all in a single spot.

Thanks regardless, I appreciate the contribution 👍

@DeveloperTheExplorer
Copy link
Author

@RobinMalfait Thanks for getting to this. I understand, I figured you guys would rather make it consistent with everything else. Happy to help with this if you need me :)

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

Successfully merging this pull request may close these issues.

3 participants