Add rejectResponderTermination prop to TextInput #11251
Conversation
|
By analyzing the blame information on this pull request, we identified @JoelMarcey and @janicduplessis to be potential reviewers. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Can anyone tell me why the Travis test failed? |
|
Re-running Travis tests |
facebook-github-bot
added
GH Review: accepted
Import Started
|
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
facebook-github-bot
added
Import Failed
and removed
Import Started
|
@facebook-github-bot shipit |
facebook-github-bot
added
the
Import Started
|
@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
facebook-github-bot
removed
the
Import Started
| @@ -492,6 +492,12 @@ const TextInput = React.createClass({ | |||
| inlineImagePadding: PropTypes.number, | |||
|
|
|||
| /** | |||
| * Allow TextInput to pass touch event to parent. | |||
| * @platform ios | |||
There was a problem hiding this comment.
Why is this iOS only? Looks like a JS change. Does it not work on Android?
There was a problem hiding this comment.
It does not make any difference on Android, where the default behaviour is for a TextInput to pass the touch event to the parent.
| @@ -492,6 +492,12 @@ const TextInput = React.createClass({ | |||
| inlineImagePadding: PropTypes.number, | |||
|
|
|||
| /** | |||
| * Allow TextInput to pass touch event to parent. | |||
There was a problem hiding this comment.
I don't understand this comment. Could you extend it please explaining how and why someone would want to use this prop?
EDIT: I understand it now but only after reading the summary in this PR. Without it people will probably not know what this doc block means.
There was a problem hiding this comment.
OK, will update the comment, thanks
|
Trying to merge again. It failed with an unrelated error. |
facebook-github-bot
added
the
Import Started
|
@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Hi guys, any movement on merging this PR? |
|
@mkonicek any news? |
|
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
|
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
I just cherry-picked and put up a new PR to try and get this merged #16755 |
One more deploy fix Circle fixes Fix deployment Improve CI config Update alias Fix Buck and tests on master Hardcode Buck version Fix Buck cache keys
This reverts commit dd016f3.
Summary: This feature was disabled for multiline textinputs in D3528202 ... seems without a good reason. The broken autoscroll-to-cursor feature is terribly harmful and counter-intuitive in ALL cases. I also add a contentSize tracking in the example app to make sure that it is unaffected by this change. https://pxl.cl/9RHP facebook#12799 facebook#15778 Special thanks to konradkierus! Reviewed By: sahrens Differential Revision: D6405985 fbshipit-source-id: 337a390a9db7b3528200ef66c4a079b87608294e
Summary: Fix the following warning: > Module RCTTVNavigationEventEmitter requires main queue setup since it overrides `init` but doesn't implement `requiresMainQueueSetup`. In a future release React Native will default to initializing all native modules on a background thread unless explicitly opted-out of. Trivial change. [IOS] [MINOR] [RCTTVNavigationEventEmitter] - Implement `requiresMainQueueSetup` in `RCTTVNavigationEventEmitter` Closes facebook#16915 Differential Revision: D6394636 Pulled By: shergin fbshipit-source-id: 3981655832715e73e93ef917d987e25097029b84
Summary: nit-picking [IOS] [MINOR] [React/Base/RCTModuleData.mm] - Fix markdown in `requiresMainQueueSetup` warning Closes facebook#16916 Differential Revision: D6394725 Pulled By: shergin fbshipit-source-id: 272c5a6ee529af0b162230e8a07275043a888907
Summary: There's a crash-inducing bug with `Image.blurRadius` on Android. `blurRadius` is specified in JavaScript as a `float`, but it's cast to `int` before being passed to the `IterativeBoxBlurPostProcessor`. However, in `IterativeBoxBlurPostProcessor`, there is an argument precondition requiring the integer `blurRadius` to be non-zero. Because the `== 0` condition is evaluated on the `float`, it's possible for a `blurRadius` in the range of `(0, 1)` (non-inclusive) to pass the conditional, and then be truncated to `0` and passed as an argument to `IterativeBoxBlurPostProcessor`, which will fail its precondition and crash the app. This change works in our app, which was previously crashing. [ANDROID] [BUGFIX] [Image] Fixed crash when specifying an Image.blurRadius between (0, 1) Closes facebook#16845 Differential Revision: D6387416 Pulled By: shergin fbshipit-source-id: d5191aa97e949ffd41e6d68c96b3c7bcbc82a52e
Summary: This change adds hooks via the `package.json` for a platform plugin to specify hooks for generating platform configurations. This change also adds hooks to allow platform plugins to participate in `link` and `unlink` commands. The goal is to move platform-specific code for platform plugins into their own repositories / node_modules. <!-- Thank you for sending the PR! We appreciate you spending the time to work on these changes. Help us understand your motivation by explaining why you decided to make this change. You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html Happy contributing! --> We need to be able to configure the CLI commands for plugin platforms (e.g., react-native-windows) in their own repositories. (Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!) - All jest tests, including new tests, pass. - `link` and `unlink` commands are successful on iOS and Android. (If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/react-native-website, and link to your PR here.) microsoft/react-native-windows#1601 <!-- Help reviewers and the release process by writing your own release notes **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.** CATEGORY [----------] TYPE [ CLI ] [-------------] LOCATION [ DOCS ] [ BREAKING ] [-------------] [ GENERAL ] [ BUGFIX ] [-{Component}-] [ INTERNAL ] [ ENHANCEMENT ] [ {File} ] [ IOS ] [ FEATURE ] [ {Directory} ] |-----------| [ ANDROID ] [ MINOR ] [ {Framework} ] - | {Message} | [----------] [-------------] [-------------] |-----------| [CATEGORY] [TYPE] [LOCATION] - MESSAGE EXAMPLES: [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see --> [CLI][FEATURE][local-cli/core/index.js] - Allow platform plugins to contribute to the RNConfig. [CLI][FEATURE][local-cli/link/link.js] - Allow platform plugins to participate in the `link` command. [CLI][FEATURE][local-cli/link/unlink.js] - Allow platform plugins to participate in the `unlink` command. Closes facebook#17745 Differential Revision: D6883558 Pulled By: hramos fbshipit-source-id: ea32fe21cedd4cc02c5c0d48229f2cdb2ac8142b
Summary: This commit removes special cases for linking iOS and Android platforms. A previous commit opened up link and other commands for other platforms to provide their own behaviors. It left special cases in tact for iOS and Android. This PR removes the special case. - Added jest tests related to the link command. - Ran the `link` and `unlink` commands for iOS and Android and confirmed no changes. facebook#17745 <!-- Help reviewers and the release process by writing your own release notes **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.** CATEGORY [----------] TYPE [ CLI ] [-------------] LOCATION [ DOCS ] [ BREAKING ] [-------------] [ GENERAL ] [ BUGFIX ] [-{Component}-] [ INTERNAL ] [ ENHANCEMENT ] [ {File} ] [ IOS ] [ FEATURE ] [ {Directory} ] |-----------| [ ANDROID ] [ MINOR ] [ {Framework} ] - | {Message} | [----------] [-------------] [-------------] |-----------| [CATEGORY] [TYPE] [LOCATION] - MESSAGE EXAMPLES: [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see --> [CLI][FEATURE][local-cli/link/link.js] - Removes special cases for linking in iOS and Android. Closes facebook#17961 Differential Revision: D6975951 Pulled By: hramos fbshipit-source-id: 8dd5da35619e2124ce4b3b18db8b694757792363
react-native-bot
added
Ran Commands
adamski
force-pushed
the
rejectResponderTerminationProp
branch
from
9638046
to
9eb9cfc
Compare
adamski
requested review from
grabbou,
hramos,
Kureev and
shergin
as code owners
|
Looks like a bad rebase here, but #16755 is a duplicate that should work. |
Summary: This is a new attempt to get #11251 merged. I just cherry-picked the relevant commits. TextInputs are set to always ignore responder termination requests, which is not desirable when they are enclosed inside a swipeable area like a ListView Create a TextInput inside a ListView and set the `rejectResponderTermination` prop to false. Otherwise, all TextInputs should have the same behavior they do now. [IOS] [ENHANCEMENT] [TextInput] - Add `rejectResponderTermination` prop to to TextInput. This enables TextInputs inside Swipeables to function properly. Pull Request resolved: #16755 Differential Revision: D7846365 Pulled By: cpojer fbshipit-source-id: eb21140061ae1f475fbd83fc63a23819e931787d
Summary: This is a new attempt to get facebook#11251 merged. I just cherry-picked the relevant commits. TextInputs are set to always ignore responder termination requests, which is not desirable when they are enclosed inside a swipeable area like a ListView Create a TextInput inside a ListView and set the `rejectResponderTermination` prop to false. Otherwise, all TextInputs should have the same behavior they do now. [IOS] [ENHANCEMENT] [TextInput] - Add `rejectResponderTermination` prop to to TextInput. This enables TextInputs inside Swipeables to function properly. Pull Request resolved: facebook#16755 Differential Revision: D7846365 Pulled By: cpojer fbshipit-source-id: eb21140061ae1f475fbd83fc63a23819e931787d
This PR exposes the
rejectResponderTerminationprop to TextInput. This allows components such as SwipeListView to be swipeable from the TextInput on iOS, as is the case on Android by default.See this article for some analysis of the problem. The author of that article created a copy-and-paste alteration of TextInput, however I wanted a more future proof version which is why I am suggesting the change in TextInput.
Tested in the iOS simulator, I'm not sure how to provide test examples for this change.
Thank you!