Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.7k
fix(onClickOutside): the order of overload signatures#4839
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| export function onClickOutside( | ||
| target: MaybeElementRef, | ||
| handler: OnClickOutsideHandler<{ detectIframe: OnClickOutsideOptions['detectIframe'], controls: true }>, | ||
| options: OnClickOutsideOptions<true>, | ||
| ): { stop: Fn, cancel: Fn, trigger: (event: Event) => void } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would you be able to assist with solving this kind of type inference issue as well? I’d really appreciate it!
onClickOutside(document.documentElement,(e)=>{// Currently, the type of e is `Event | PointerEvent`// The correct type should be `Event | PointerEvent | FocusEvent`.},{controls:true,detectIframe:true})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok. i'll see what i can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had to add stricter overloads for the handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we change it like this?
-export interface OnClickOutsideOptions<Controls extends boolean = false> extends ConfigurableWindow {+export interface OnClickOutsideOptions<Controls extends boolean = false, DetectIframe extends boolean = false> extends ConfigurableWindow { // ...- detectIframe?: boolean+ detectIframe?: DetectIframe // ...}
43081j commentedJun 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
i think you could simplify this to two overloads 👀 you only need to know what so we could instead do something like this: exportfunctiononClickOutside<TextendsOnClickOutsideOptions<true>>(target:MaybeElementRef,handler:OnClickOutsideHandler<T>,options:T):Controls;exportfunctiononClickOutside<TextendsOnClickOutsideOptions<false>>(target:MaybeElementRef,handler:OnClickOutsideHandler<T>,options:T):Fn; and exporttypeOnClickOutsideHandler<TControlsextendsboolean,// may not be needed? may be able to just do `<boolean>` belowTextendsOnClickOutsideOptions<TControls>>=(event:(T['detectIframe']extendstrue ?FocusEvent :never)|(T['controls']extendstrue ?Event :never)|PointerEvent,)=>void |
Before submitting the PR, please make sure you do the following
fixes #123).Description
Fixes#4838.
The order of Overload Signatures has been changed.
Additional context