feat: Add onBeforeToggle event handler type #4694
Conversation
lukewarlow
force-pushed
the
onbeforetoggle-types
branch
from
d60366b to
c33c424
Compare
| @@ -625,7 +625,8 @@ export namespace JSXInternal { | |||
| onCompositionUpdate?: CompositionEventHandler<Target> | undefined; | |||
| onCompositionUpdateCapture?: CompositionEventHandler<Target> | undefined; | |||
|
|
|||
| // Details Events | |||
| // Popover Events | |||
| onBeforeToggle?: ToggleEventHandler<Target> | undefined; | |||
| onToggle?: ToggleEventHandler<Target> | undefined; | |||
There was a problem hiding this comment.
DetailsHTMLAttributes has an onToggle defined too, but as a GenericEventHandler idk if that should be removed or if it is in addition to this type (can't really remember how TS works).
There was a problem hiding this comment.
That'd be a mistake -- IIRC, the DetailsHTMLAttributes interface was created prior, then moved into this module. Later we added this entry with a backport of the correct event handler type. I think the DetailsHTML interface will take precedence too so that should probably be removed in favor of this "global" type, but that can be done separately later.
I think in the future we'll try to break up this mega list of events too, try to restrict them (where appropriate) to specific elements.
This is slightly complicated, basically Details has had an ontoggle for a long time but historically it was just an Event. More recently Popovers were added and they had a beforetoggle and a toggle event but used a new ToggleEvent type. Details was subsequently updated to use this type for it's toggle event too (but it doesn't have a beforetoggle event itself).
Dialogs also more recently have had a toggle and beforetoggle event added to them.