[mergeProps] Make mergeProps public
#3642
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
michaldudak
force-pushed
the
document-merge-props
branch
from
02ece77 to
d49021f
Compare
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
LGTM. 👍
Thanks for addressing this. 🙏
Co-authored-by: Lukas Tyla <llukas.tyla@gmail.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>
| ...props | ||
| }: ParametersReferenceTableProps) { | ||
| return ( | ||
| <PropsReferenceAccordion |
There was a problem hiding this comment.
Would it make sense to abstract this into a <ReferenceAccordion/> component which both the parameters and props table would use?
| ```ts | ||
| mergeProps({ id: 'a', dir: 'ltr' }, { id: 'b' }); // returns { id: 'b', dir: 'ltr' } | ||
| ``` | ||
| Note that this includes refs. |
There was a problem hiding this comment.
I don't think this is the behavior most people would expect without diving deeper into why we don't merge the refs.
So I think it's worth being extra-explicit here and add a bullet point specifically for the ref 👍
| Each argument can be a props object or a function that receives the merged props up to that point (left to right) and returns a props object. | ||
| This is useful when you need to compute the next props from whatever has already been merged. | ||
|
|
||
| When you return event handlers from a function argument, `preventBaseUIHandler()` is not applied automatically to those handlers. |
There was a problem hiding this comment.
A couple of code snippets on how to use the baseUIHandlerPrevented and preventBaseUIHandler() could be nice (with an object and with a function)
| - For React synthetic events, Base UI adds `event.preventBaseUIHandler()`. Calling it sets `event.baseUIHandlerPrevented = true`. | ||
| This does not call `preventDefault()` or `stopPropagation()`. | ||
| It only tells Base UI to skip earlier merged handlers or internal logic that checks the flag. | ||
| - For non-synthetic events (custom events with primitive/object values), this mechanism isn't available and all handlers always execute. |
There was a problem hiding this comment.
There's extra indentation here
| ```ts | ||
| mergeProps({ onClick: a }, { onClick: b }); // b runs before a | ||
| ``` |
There was a problem hiding this comment.
The code blocks are missing a title, maybe the comments could be moved to the title
github-actions
bot
added
the
PR: out-of-date
6 participants
mergePropswas already exported from the package, but not documented. As they are useful (shadcn components use it, for example), I added the docs page for them.The docs were generated using an updated infrastructure, so the API definitions are taken from JSDocs (as in components) and not typed by hand.
Docs: https://deploy-preview-3642--base-ui.netlify.app/react/utils/merge-props