[core] Make mergeReactProps work with non-native event handlers
#1440
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| ); | ||
| } | ||
|
|
||
| function mergeEventHandlers(theirHandler: Function, ourHandler: Function) { | ||
| return (event: unknown) => { |
There was a problem hiding this comment.
@michaldudak I was wondering if this was a valid use-case: if
you use useRender (or Slot) to create a custom component
with event handlers that match ours like this, and want to conditionally cancel "our" handler:
<MySlider
onValueChange={(_value, event) => {
if (/* something */) {
event.preventBaseUIDefault()
}
}}
render={
<Slider.Root />
}
/>But I realized (generally) <NumberField.Root onValueChange={(_value, event) => { event.preventBaseUIDefault() }} /> isn't possible either
There was a problem hiding this comment.
This API was designed to handle native events only. It's meant to prevent our logic from running on DOM elements (which only accept native events). If there's a real-world scenario that requires skipping logic in our custom handlers, we may revisit it. Most (if not all) of our events are called in reaction to something happening, and there's nothing to prevent. Considering the example you posted: onValueChange is called as a last step of setValue, so it can't prevent any code from being run.
There was a problem hiding this comment.
Looks good ~ I tested it with the original sandbox: https://codesandbox.io/p/devbox/agitated-darkness-djktpw?file=%2Fsrc%2FApp.tsx
I suppose we can revisit the issue of merging and cancelling our custom handlers once #1418 is completed
Fixed the
mergeReactPropsutility to handle event handlers that don't have the event as the first parameter. It now checks if the parameter is an object and contains thenativeEventfield before adding thepreventBaseUIHandlermethod.Additionally, I refactored the implementation to be more readable.
Fixes #1430