-
Notifications
You must be signed in to change notification settings - Fork 47.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
Add support for "invalid" event within Form elements #5187
Add support for "invalid" event within Form elements #5187
Conversation
@@ -687,6 +698,19 @@ ReactDOMComponent.Mixin = { | |||
}, | |||
|
|||
/** | |||
* Setup this component to trap non-bubbling events locally |
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'm not sure if this description suffices but I didn't know how else to describe what it does!
Feedback welcome on approach and on how to unit/integration test this |
I don't see anything wrong here; looks good to me. @tomduncalf Did you test it manually in a browser to verify it works? It is possible to write tests that fire mocked events, but honestly it's pretty fragile. |
Yes, tested manually - can share the test case if you like. Couldn't see anywhere obvious to hook into unit testing it!
|
Ok, looks good to me. Thanks @tomduncalf! |
…5152 Add support for "invalid" event within Form elements
this._wrapperState = { | ||
listeners: null, | ||
}; | ||
transaction.getReactMountReady().enqueue(trapBubbledEventsLocal, this); |
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.
The mountWrapper
calls in input, select, and textarea should include the listeners
property in their _wrapperState
to avoid changing the hidden class when the assignment is made. This would also be better just as a free function – we're trying to get away from OO in the core anyway.
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.
@spicyj Makes sense - probably worth raising a separate issue for this as this PR is now merged
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 know it's merged. I was hoping that one of you would want to post a PR to fix it but I did it myself in #5213.
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.
Cool - thank you!
Fixes #5152