-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix: Scroll to picker regression #14409
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
Changes from 13 commits
c4e8ea8
71a415a
384eedb
1cbeaa7
fe8d4cd
65d5e9c
7ba22bd
8172dce
8edc865
3505fd9
b7ff191
8cba034
27f5fea
165af16
7d59c4b
b08dc2b
8c479d2
a0e9574
bf7f0da
c37e279
63ff50a
3392ff1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import lodashGet from 'lodash/get'; | ||
| import React from 'react'; | ||
| import {View} from 'react-native'; | ||
| import {View, ScrollView} from 'react-native'; | ||
| import PropTypes from 'prop-types'; | ||
| import _ from 'underscore'; | ||
| import {withOnyx} from 'react-native-onyx'; | ||
|
|
@@ -56,6 +56,18 @@ const propTypes = { | |
| /** Whether the form submit action is dangerous */ | ||
| isSubmitActionDangerous: PropTypes.bool, | ||
|
|
||
| /** Defines wheter the overflow content of the form's ScrollView should be scrollable | ||
| * Should be set to true, when there's a nested Picker component relatively at the end of | ||
| * the ScrollView's content, so the opening Picker modal doesn't get rendered over the Picker | ||
| * Example can be found here: https://github.com/Expensify/App/issues/13909#issuecomment-1396859008 | ||
| */ | ||
chrispader marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| scrollToOverflowEnabled: PropTypes.bool, | ||
|
|
||
| /** Defines wheter ScrollWithContext should be used instead of regular ScrollView | ||
| * Should be set to true, when there's a nested Picker component in the ScrollView's children | ||
| */ | ||
chrispader marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| scrollContextEnabled: PropTypes.bool, | ||
luacmartins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ...withLocalizePropTypes, | ||
| }; | ||
|
|
||
|
|
@@ -68,6 +80,8 @@ const defaultProps = { | |
| draftValues: {}, | ||
| enabledWhenOffline: false, | ||
| isSubmitActionDangerous: false, | ||
| scrollToOverflowEnabled: false, | ||
| scrollContextEnabled: false, | ||
| }; | ||
|
|
||
| class Form extends React.Component { | ||
|
|
@@ -79,6 +93,7 @@ class Form extends React.Component { | |
| inputValues: {}, | ||
| }; | ||
|
|
||
| this.formRef = React.createRef(null); | ||
| this.inputRefs = {}; | ||
| this.touchedInputs = {}; | ||
|
|
||
|
|
@@ -258,45 +273,60 @@ class Form extends React.Component { | |
| } | ||
|
|
||
| render() { | ||
| const scrollViewContent = safeAreaPaddingBottomStyle => ( | ||
| <View style={[this.props.style, safeAreaPaddingBottomStyle]}> | ||
| {this.childrenWrapperWithProps(this.props.children)} | ||
| {this.props.isSubmitButtonVisible && ( | ||
| <FormAlertWithSubmitButton | ||
| buttonText={this.props.submitButtonText} | ||
| isAlertVisible={_.size(this.state.errors) > 0 || Boolean(this.getErrorMessage()) || !_.isEmpty(this.props.formState.errorFields)} | ||
| isLoading={this.props.formState.isLoading} | ||
| message={_.isEmpty(this.props.formState.errorFields) ? this.getErrorMessage() : null} | ||
| onSubmit={this.submit} | ||
| onFixTheErrorsLinkPressed={() => { | ||
| const errors = !_.isEmpty(this.state.errors) ? this.state.errors : this.props.formState.errorFields; | ||
| const focusKey = _.find(_.keys(this.inputRefs), key => _.keys(errors).includes(key)); | ||
| const focusInput = this.inputRefs[focusKey]; | ||
| if (focusInput.focus && typeof focusInput.focus === 'function') { | ||
| focusInput.focus(); | ||
| } | ||
|
|
||
| // We subtract 10 to scroll slightly above the input | ||
| if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') { | ||
| focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false})); | ||
| } | ||
| }} | ||
| containerStyles={[styles.mh0, styles.mt5, styles.flex1]} | ||
| enabledWhenOffline={this.props.enabledWhenOffline} | ||
| isSubmitActionDangerous={this.props.isSubmitActionDangerous} | ||
| /> | ||
| )} | ||
| </View> | ||
| ); | ||
|
|
||
| return ( | ||
| <SafeAreaConsumer> | ||
| {({safeAreaPaddingBottomStyle}) => ( | ||
| {({safeAreaPaddingBottomStyle}) => (this.props.scrollContextEnabled ? ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aldo-expensify, @chrispader Here could've been a better implementation instead of DRY code , something like const ScrollViewComponent = this.props.scrollContextEnabled ? ScrollViewWithContext : ScrollView;
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, that would have resulted in nicer code 🥲 |
||
| <ScrollViewWithContext | ||
| style={[styles.w100, styles.flex1]} | ||
| contentContainerStyle={styles.flexGrow1} | ||
| keyboardShouldPersistTaps="handled" | ||
| ref={el => this.form = el} | ||
| scrollToOverflowEnabled={this.props.scrollToOverflowEnabled} | ||
| ref={this.formRef} | ||
| > | ||
| <View style={[this.props.style, safeAreaPaddingBottomStyle]}> | ||
| {this.childrenWrapperWithProps(this.props.children)} | ||
| {this.props.isSubmitButtonVisible && ( | ||
| <FormAlertWithSubmitButton | ||
| buttonText={this.props.submitButtonText} | ||
| isAlertVisible={_.size(this.state.errors) > 0 || Boolean(this.getErrorMessage()) || !_.isEmpty(this.props.formState.errorFields)} | ||
| isLoading={this.props.formState.isLoading} | ||
| message={_.isEmpty(this.props.formState.errorFields) ? this.getErrorMessage() : null} | ||
| onSubmit={this.submit} | ||
| onFixTheErrorsLinkPressed={() => { | ||
| const errors = !_.isEmpty(this.state.errors) ? this.state.errors : this.props.formState.errorFields; | ||
| const focusKey = _.find(_.keys(this.inputRefs), key => _.keys(errors).includes(key)); | ||
| const focusInput = this.inputRefs[focusKey]; | ||
| if (focusInput.focus && typeof focusInput.focus === 'function') { | ||
| focusInput.focus(); | ||
| } | ||
|
|
||
| // We subtract 10 to scroll slightly above the input | ||
| if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') { | ||
| focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false})); | ||
| } | ||
| }} | ||
| containerStyles={[styles.mh0, styles.mt5, styles.flex1]} | ||
| enabledWhenOffline={this.props.enabledWhenOffline} | ||
| isSubmitActionDangerous={this.props.isSubmitActionDangerous} | ||
| /> | ||
| )} | ||
| </View> | ||
| {scrollViewContent(safeAreaPaddingBottomStyle)} | ||
| </ScrollViewWithContext> | ||
| )} | ||
| ) : ( | ||
| <ScrollView | ||
| style={[styles.w100, styles.flex1]} | ||
| contentContainerStyle={styles.flexGrow1} | ||
| keyboardShouldPersistTaps="handled" | ||
| scrollToOverflowEnabled={this.props.scrollToOverflowEnabled} | ||
| ref={this.formRef} | ||
| > | ||
| {scrollViewContent(safeAreaPaddingBottomStyle)} | ||
| </ScrollView> | ||
| ))} | ||
| </SafeAreaConsumer> | ||
| ); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.