Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[flow-strict] Flow strict TextProps #22122

Closed
wants to merge 14 commits into from
Closed

[flow-strict] Flow strict TextProps #22122

wants to merge 14 commits into from

Conversation

exced
Copy link
Contributor

@exced exced commented Nov 4, 2018

Related to #22100

Turn on Flow strict mode for TextProps.
I used ResponseHandlers type definition defined in Text.js.

I wanted to move ResponseHandlers type to TextProps and reuse it inside the file.
I know I could use $Shape<> to maybe keys but how do I elegantly maybe every values ?
Unless having a straightforward solution, I found it clearer to copy paste these types.

Test Plan:

  • All flow tests succeed.

Release Notes:

[GENERAL] [ENHANCEMENT] [TextProps.js] - Flow strict mode

@exced exced requested a review from shergin as a code owner November 4, 2018 23:05
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2018
@turnrye
Copy link
Contributor

turnrye commented Nov 5, 2018

Hi @exced! Thanks for the help! I plan to combine those entries in the changelog to a single entry, so the changelog section in this PR is n/a. I'll make sure that once this PR lands, you're attributed for your contribution.

@RSNara RSNara changed the title Flow strict TextProps [flow-strict] Flow strict TextProps Nov 5, 2018
@TheSavior
Copy link
Member

cc @empyrical, you two have similar types here for the responders between ViewProps and TextProps. Can you work together to figure out how these types should be consistent / share the same types?

@empyrical
Copy link
Contributor

Maybe PanResponder can export a ResponderProps type that both View and Text can use the ...spread operator to make use of them?

@exced
Copy link
Contributor Author

exced commented Nov 5, 2018

@empyrical Sounds good, do you want me to add it into this PR ?

@empyrical
Copy link
Contributor

empyrical commented Nov 5, 2018 •
edited

Actually I'm not sure if this can be done - it looks like Text and View don't all use the same responder props.

But for the type of event on your callbacks, you should import and use GestureState from PanResponder. I realize I improperly typed the event type in my PR and I will be updating them too. Then I think we are good on the responder types!

@empyrical
Copy link
Contributor

Sorry ignore that suggestion, I had that wrong because I was looking at something else 🤦‍♂️ Import PressEvent from CoreEventTypes and use that instead of SyntheticEvent<>

@exced
Copy link
Contributor Author

exced commented Nov 5, 2018

No worries 😊

@exced
Copy link
Contributor Author

exced commented Nov 6, 2018

@empyrical I replaced SyntheticEvent<> by PressEvent

RSNara
RSNara approved these changes Nov 14, 2018
View reviewed changes
Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 😁

facebook-github-bot
facebook-github-bot reviewed Nov 14, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@exced exced mentioned this pull request Nov 15, 2018
Closed
28 tasks
@react-native-bot
Copy link
Collaborator

@exced merged commit 7927497 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Nov 21, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 21, 2018
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
@exced @facebook-github-bot
Flow strict TextProps (facebook#22122)
f9a11bb 
Summary:
Related to facebook#22100

Turn on Flow strict mode for TextProps.
I used ResponseHandlers type definition defined in Text.js.

I wanted to move ResponseHandlers type to TextProps and reuse it inside the file.
I know I could use $Shape<> to maybe keys but how do I elegantly maybe every values ?
Unless having a straightforward solution, I found it clearer to copy paste these types.

- All flow tests succeed.

[GENERAL] [ENHANCEMENT] [TextProps.js] - Flow strict mode
Pull Request resolved: facebook#22122

Reviewed By: TheSavior

Differential Revision: D13055759

Pulled By: RSNara

fbshipit-source-id: 230b43c7c94d7f82f5727ad11541b0cb98bc5e3a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@RSNara RSNara RSNara approved these changes

@shergin shergin Awaiting requested review from shergin

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: Text Flow Merged This PR has been merged.
Projects
No open projects
🎁 Open Sourcing Internals
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@exced @turnrye @TheSavior @empyrical @react-native-bot @RSNara @facebook-github-bot