-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[flow-strict] Flow strict ScrollResponder #22181
Conversation
Generated by 🚫 dangerJS |
@@ -113,7 +114,16 @@ type State = { | |||
observedScrollSinceBecomingResponder: boolean, | |||
becameResponderWhileAnimating: boolean, | |||
}; | |||
type Event = Object; | |||
type Event = $ReadOnly<{| |
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.
Is SyntheticEvent
from CoreEventTypes
helpful here?
Also, are we sure this shouldn't be ScrollEvent
from CoreEventTypes
? Where do you see that this event contains touches?
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.
In response to your review, I changed PR a lot.
I found that the event defined by the existing flow type is getting passed in the arguments of all the functions defined in ScrollResponder
, so we don't need to use SysntheticEvent
here.
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.
Looks good! 🤔
@@ -584,8 +588,8 @@ const ScrollResponderMixin = { | |||
this.preventNegativeScrollOffset = false; | |||
}, | |||
|
|||
scrollResponderTextInputFocusError: function(e: Event) { | |||
console.error('Error measuring text field: ', e); | |||
scrollResponderTextInputFocusError: function(msg: string) { |
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.
Was this function always being called with a string
?
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.
Yes, the function is finally passed to UIManager.measureLayout
as error callback.
In Java, the error callback argument is string, and in Objective-C, the callback is never called.
ref:
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.
@RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Related to facebook#22100 Enhance Flow types for Libraries/Components/ScrollResponder.js Pull Request resolved: facebook#22181 Reviewed By: TheSavior Differential Revision: D13032699 Pulled By: RSNara fbshipit-source-id: ca0ce178a96008a71016d033ee1154ad807d6599
Related to #22100
Enhance Flow types for Libraries/Components/ScrollResponder.js
Test Plan:
Changelog:
[GENERAL] [ENHANCEMENT] [ScrollResponder.js] - Flow types