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.

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

Bring back TextInput.State, deprecate focusTextInput and blurTextInput #18936

Closed

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Apr 19, 2018

a275eac removed TextInput.State but we should keep it as it was a public-ish API and we don't have any migration plan off it. Also bring back focusTextInput and blurTextInput with a deprecation warning.

Test Plan

Tested TextInput.State is back

Release Notes

[GENERAL][ENHANCEMENT][TextInput] - Bring back TextInput.State, deprecate focusTextInput and blurTextInput

@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 Apr 19, 2018
statics: {
State: {
currentlyFocusedField: TextInputState.currentlyFocusedField,
focusTextInput: TextInputState.focusTextInput,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also kept those 2 as I suspect it might break things if we remove it without warning. Not sure if we want to deprecate since there are no real alternative except using TextInput refs for focus / blur but some people might use it outside the component tree...

Copy link
Contributor

Choose a reason for hiding this comment

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

The proper way is to call .focus() or .blur() on the ref. How would you even get a node handle for it? I'd rather not add this back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is usually used in combination with currentlyFocusedField, which is the node handle. There is also findNodeHandle. I agree the other API is preferable, but there might be valid use cases for TextInput.State one, like the keyboard library I noticed the issue in. If we still want to remove this we should probably deprecate it for a release cycle first.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have Keyboard.dismiss (or dismissKeyboard) for blurring the focused field.

If you will take care of deprecating it then removing it in the next release, we can add it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, The only one that there is no alternative currently is currentlyFocusedField, which is useful to measure where the focused the input is in the screen (example: https://github.com/APSL/react-native-keyboard-aware-scroll-view/blob/master/lib/KeyboardAwareHOC.js#L267). I will add a deprecation warning to the other 2 methods.

@react-native-bot react-native-bot added Core Team Component: TextInput Related to the TextInput component. labels Apr 19, 2018
@janicduplessis
Copy link
Contributor Author

@sophiebits Could you have a look a this?

Copy link
Contributor

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

Can you split this into two changes in case we need to revert the ScrollResponder.js one?

statics: {
State: {
currentlyFocusedField: TextInputState.currentlyFocusedField,
focusTextInput: TextInputState.focusTextInput,
Copy link
Contributor

Choose a reason for hiding this comment

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

The proper way is to call .focus() or .blur() on the ref. How would you even get a node handle for it? I'd rather not add this back.

@janicduplessis
Copy link
Contributor Author

Split the PR, this is now only the TextInput.State part, see #19255 for the other.

@sophiebits
Copy link
Contributor

👍 thanks!

@janicduplessis janicduplessis changed the title Bring back TextInput.State, restore isTextInput check Bring back TextInput.State, deprecate focusTextInput and blurTextInput May 14, 2018
@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label May 17, 2018
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.

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

macdoum1 pushed a commit to macdoum1/react-native that referenced this pull request Jun 28, 2018
Summary:
a275eac removed TextInput.State but we should keep it as it was a public-ish API and we don't have any migration plan off it. Also bring back `focusTextInput` and `blurTextInput` with a deprecation warning.

Tested TextInput.State is back

[GENERAL][ENHANCEMENT][TextInput] - Bring back TextInput.State, deprecate focusTextInput and blurTextInput
Closes facebook#18936

Differential Revision: D8044439

Pulled By: hramos

fbshipit-source-id: fde145f04bb1d46ef58b5954cb7963adf495b21c
facebook-github-bot pushed a commit that referenced this pull request Aug 6, 2018
…extInput (#20326)

Summary:
In #18936 we decided to deprecate `focusTextInput` and `blurTextInput` but since then I found a valid use case for it that is pretty much impossible to implement otherwise.

React Navigation uses it to blur / re-focus the input during the swipe back gesture. Blur can be done with Keyboard.dismiss but without this api we cannot re-focus the text field that was focused if the swipe back gesture is cancelled. See https://github.com/react-navigation/react-navigation/blob/master/src/navigators/createKeyboardAwareNavigator.js#L21-L34

I think it is best to just bring back this api.
Pull Request resolved: #20326

Differential Revision: D9182810

Pulled By: hramos

fbshipit-source-id: 3740421ffafb8f814522d15788f3466324177c16
kelset pushed a commit that referenced this pull request Aug 13, 2018
…extInput (#20326)

Summary:
In #18936 we decided to deprecate `focusTextInput` and `blurTextInput` but since then I found a valid use case for it that is pretty much impossible to implement otherwise.

React Navigation uses it to blur / re-focus the input during the swipe back gesture. Blur can be done with Keyboard.dismiss but without this api we cannot re-focus the text field that was focused if the swipe back gesture is cancelled. See https://github.com/react-navigation/react-navigation/blob/master/src/navigators/createKeyboardAwareNavigator.js#L21-L34

I think it is best to just bring back this api.
Pull Request resolved: #20326

Differential Revision: D9182810

Pulled By: hramos

fbshipit-source-id: 3740421ffafb8f814522d15788f3466324177c16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TextInput Related to the TextInput component. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants