-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
components: Add mergeEventHandlers #33205
Conversation
Size Change: +13.8 kB (+1%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
Something I noticed about this utility is that it creates an intersection of the left and right values + the rest of the left:
I'm not sure this is the correct behavior... it seems like we should get back |
This is what I'd expect too — I've also added a related comment which highlights where this potential bug may come from. Is there any real usage of this function, just to make sure that we're making the correct assumption about the way it should behave? |
Update pushed should address the issues. |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
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.
LGTM 🚀
* components: Add mergeEventHandlers * Preserve the right hand sides non-interescting properties * Simplify merging logic * Update packages/components/src/utils/events.ts Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Description
Adds
mergeEventHandlers
function that is needed by the TextInput component.How has this been tested?
Unit test should pass.
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).