Add rejectResponderTermination prop to TextInput #16755
Conversation
|
Thanks @cmcewen, would be good to see this go in finally! |
|
@hramos this was originally approved a while ago and failed on import, and it also doesn't change the default behavior at all. Any chance we can get this merged? |
|
@cmcewen I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
I would really appreciated if this PR could be merged soon. It was first submitted as #11251 over a year ago. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions. |
|
This is a super simple PR just providing access to a property but not changing the default behavior at all |
|
Yes, it is a simple enough change, would be great to see it go in soon. |
|
Can't believe a such simple PR can't be merged since Dec 1, 2016 |
|
This may appear simple but can have side effects on some of our internal apps. I'll let @shergin take another look, since he had some concerns when he last looked at the related PR. |
|
In any case, the PR will need to be rebased since it has been awhile since it was opened. As a bonus, we can ensure tests pass. |
d5987d7 to
3257688
Compare
145b634 to
5dec8b8
Compare
|
@hramos rebased + tests passing |
|
@hramos the tests are passing and the PR was rebased - anything else I can do to get this in? |
|
@cmcewen looks like this is still waiting on a review as I mentioned earlier. |
|
@cmcewen I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
There was a problem hiding this comment.
@shergin is landing 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. |
|
Not sure if I'm able to rebase this now that it's become part of a newer PR. In any case, I hope it can be merged soon. |
5dec8b8 to
4b27a7d
Compare
|
Let's actually land this for real. Sorry for the long wait. |
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Generated by 🚫 dangerJS |
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
Motivation
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
Test Plan
Create a TextInput inside a ListView and set the
rejectResponderTerminationprop to false. Otherwise, all TextInputs should have the same behavior they do now.Release Notes
[IOS] [ENHANCEMENT] [TextInput] - Add
rejectResponderTerminationprop to to TextInput. This enables TextInputs inside Swipeables to function properly.