-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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 additional event API responder surfaces #15248
Conversation
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 700f17b...6557cfa react-dom
react-events
Generated by 🚫 dangerJS |
👀 I really like how this is shaping up. |
import {REACT_EVENT_COMPONENT_TYPE} from 'shared/ReactSymbols'; | ||
|
||
const targetEventTypes = ['pointerdown', 'pointercancel']; | ||
const rootEventTypes = ['pointerup', {name: 'pointermove', passive: false}]; |
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 think we could document why certain events are not passive throughout these event module implementations.
listener: (e: Object) => void, | ||
state: DragState, | ||
discrete: boolean, | ||
eventData?: { |
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.
We should figure out the event shapes we want for these events. Drag will probably need the delta, coordinates in space, offset from initial position, and velocity.
|
||
if ( | ||
props.onShouldClaimOwnership && | ||
props.onShouldClaimOwnership() |
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.
This prop name doesn't seem quite right. In the RN responder system there are onStartShouldSetResponder
and onMoveShouldSetResponder
props to determine whether the view wants to become the responder after a start or move event.
} | ||
break; | ||
} | ||
case 'pointercancel': |
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.
we might eventually want a separate event to be called if the drag is cancelled
import {REACT_EVENT_COMPONENT_TYPE} from 'shared/ReactSymbols'; | ||
|
||
const targetEventTypes = [ | ||
{name: 'focus', passive: true, capture: true}, |
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.
curious why these need to be passive and capture
if (props.onSwipeLeft && direction === 3) { | ||
dispatchSwipeEvent( | ||
context, | ||
'swipeleft', |
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 think it might be preferable to have a single event like onSwipe
with event data that provides the direction, velocity, etc. That would be simpler to work with in userland than potentially listening to 4 events.
Note: this is for an experimental event API that we're testing out internally at Facebook.
This PR adds more event responder surfaces, fills in the TODOs around the responder ownership model and also fixes some bugs found with internal user testing.