Don't use setState for disabled KeyboardAvoidingView to avoid re-renders #38074
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: 0bd6b28 |
|
Looks like you've got flow test failing, can you take a look? |
| _setBottom = (value: number) => { | ||
| const {enabled = true} = this.props; | ||
| this._bottom = value; | ||
| if (enabled === true) { |
There was a problem hiding this comment.
| if (enabled === true) { | |
| if (enabled) { |
Self explanatory, though stylistic :)
There was a problem hiding this comment.
@adamgrzybowski I see you made that change on your 2nd commit. What was your thinking?
There was a problem hiding this comment.
This is why the flow tests were failing. For some reason if(enabled) is not enough with const {enabled = true} = this.props
Sketchy null check on boolean [1] which is potentially false. Perhaps you meant to check for null or undefined [2]?
[sketchy-null-bool]
[2][1] 45│ enabled?: ?boolean,
:
134│ _setBottom = (value: number) => {
135│ const {enabled = true} = this.props;
136│ this._bottom = value;
137│ if (enabled) {
138│ this.setState({bottom: value});
139│ }
140│ };
But to be honest I am not convinced if this is the right way.
I just checked different approach and it works fine for
const enabled = this.props.enabled ?? true;
if (enabled) { ... }I could switch to this syntax to make it more readable. What do you think?
There was a problem hiding this comment.
@blakef @Pranav-yadav done
packages/react-native/Libraries/Components/Keyboard/KeyboardAvoidingView.js
Outdated
Show resolved
Hide resolved
packages/react-native/Libraries/Components/Keyboard/KeyboardAvoidingView.js
Outdated
Show resolved
Hide resolved
|
This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days |
github-actions
bot
added
the
Stale
|
Is there anything else I should answer? @blakef @Pranav-yadav |
github-actions
bot
removed
the
Stale
|
Hi, what's the status on this PR? Is it ready to be merged? |
|
This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days |
github-actions
bot
added
the
Stale
|
I think it still waits for approve |
github-actions
bot
removed
the
Stale
|
given that checks are all passing, @cortinico maybe you can help find someone from Meta to get this in? |
|
@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was successfully merged by @adamgrzybowski in 783150f. When will my fix make it into a release? | Upcoming Releases |
…ers (facebook#38074) Summary: There are two reasons to apply these changes: - We don't need to re-render the `KeyboardAvoidingView` if it is disabled. It may be especially useful in combination with [react-navigation](https://reactnavigation.org/) where we could disable `KeyboardAvoidingView` for screens that are not focused - They fix the problem with the `KeyboardAvoidingView` wrapped inside the [react-freeze](https://github.com/software-mansion/react-freeze) component. Similarly, as above, it is useful when we want to freeze screens that are not visible for the user. ## Changelog: [GENERAL] [CHANGED] Don't use setState for disabled KeyboardAvoidingView to avoid re-renders <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#38074 Test Plan: - Check if the KeyboardAvoidingView works as expected. Reviewed By: sammy-SC Differential Revision: D49148391 Pulled By: blakef fbshipit-source-id: c4b7bde696d2249cbf4ad12c77058183b632464d
|
Hello @adamgrzybowski, I upgraded all the dependencies and it did not fix it. No error is sent on focus to Sentry although I asked for one to be sent onFocus and onChangeText. I can't add anything with the keyboard. Although, in some cases, I can type something with the keyboard but it freezes 2 or 3 seconds later... I can't see any difference between when it works and when it does not. This is my package.json, could you help me debug this? 🙏 |
Summary:
There are two reasons to apply these changes:
KeyboardAvoidingViewif it is disabled. It may be especially useful in combination with react-navigation where we could disableKeyboardAvoidingViewfor screens that are not focusedKeyboardAvoidingViewwrapped inside the react-freeze component. Similarly, as above, it is useful when we want to freeze screens that are not visible for the user.Changelog:
[GENERAL] [CHANGED] Don't use setState for disabled KeyboardAvoidingView to avoid re-renders
Test Plan: