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

Fix: Scroll to picker regression #14409

Merged
merged 22 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c4e8ea8
fix: add missing forwardRef to ScrollViewWithContext
Jan 19, 2023
71a415a
feat: Introduce scrollContextEnabled and scrollToOverflowEnabled prop…
Jan 19, 2023
384eedb
fix: use scrollToOverflowEnabled prop from passed down props
Jan 19, 2023
1cbeaa7
Use scrollContext and overflow on AddDebitCardPage
Jan 19, 2023
fe8d4cd
fix: remove random string in Form
Jan 19, 2023
65d5e9c
fix: usage of new ref in Form
Jan 19, 2023
7ba22bd
Merge branch 'main' into fix/scroll-to-picker-regression
Jan 19, 2023
8172dce
Merge branch 'main' into fix/scroll-to-picker-regression
Jan 20, 2023
8edc865
fix: add explanation for two Form props
Jan 20, 2023
3505fd9
doc: add docs on how to handle pickers in forms
Jan 20, 2023
b7ff191
add picker scroll functionality to CompanyStep
Jan 20, 2023
8cba034
fix: add picker scroll functionality to AdditionalDetailsStep
Jan 20, 2023
27f5fea
fix: add picker scroll functionality to (currently) unused screens
Jan 20, 2023
165af16
Merge branch 'main' into fix/scroll-to-picker-regression
Jan 23, 2023
7d59c4b
Update src/components/Form.js
chrispader Jan 23, 2023
b08dc2b
Update src/components/Form.js
chrispader Jan 23, 2023
8c479d2
Update contributingGuides/FORMS.md
chrispader Jan 23, 2023
a0e9574
fix: add scrollContextEnabled to missing screens
Jan 23, 2023
bf7f0da
update comment
Jan 23, 2023
c37e279
Update src/components/Form.js
chrispader Jan 24, 2023
63ff50a
fix: passed invalid ref to measureLayout
Jan 24, 2023
3392ff1
Merge branch 'main' into fix/scroll-to-picker-regression
Jan 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions contributingGuides/FORMS.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,15 @@ Any `Form.js` that has a button will also add safe area padding by default. If t
</Form>
</ScreenWrapper>
```

### Handling nested Pickers in Form

In case there's a nested Picker in Form, we should pass the props below to Form, as needed:

#### Enable ScrollContext

Pass the `scrollContextEnabled` prop to enable scrolling up when Picker is pressed, making sure the Picker is always in view and doesn't get covered by virtual keyboards for example.

#### Enable scrolling to overflow

In addition to the `scrollContextEnabled` prop, we can also pass `scrollToOverflowEnabled` when the nested Picker is at the bottom of the Form to prevent the popup selector from covering Picker.
93 changes: 61 additions & 32 deletions src/components/Form.js
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';
Expand Down Expand Up @@ -56,6 +56,17 @@ const propTypes = {
/** Whether the form submit action is dangerous */
isSubmitActionDangerous: PropTypes.bool,

/** Whether the ScrollView overflow content is scrollable.
* Set to true to avoid nested Picker components at the bottom of the Form from rendering the popup selector over Picker
* e.g. https://github.com/Expensify/App/issues/13909#issuecomment-1396859008
*/
scrollToOverflowEnabled: PropTypes.bool,

/** Whether ScrollWithContext should be used instead of regular ScrollView.
* Set to true when there's a nested Picker component in Form.
*/
scrollContextEnabled: PropTypes.bool,
luacmartins marked this conversation as resolved.
Show resolved Hide resolved

...withLocalizePropTypes,
};

Expand All @@ -68,6 +79,8 @@ const defaultProps = {
draftValues: {},
enabledWhenOffline: false,
isSubmitActionDangerous: false,
scrollToOverflowEnabled: false,
scrollContextEnabled: false,
};

class Form extends React.Component {
Expand All @@ -79,6 +92,7 @@ class Form extends React.Component {
inputValues: {},
};

this.formRef = React.createRef(null);
this.inputRefs = {};
this.touchedInputs = {};

Expand Down Expand Up @@ -258,45 +272,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.formRef.current, (x, y) => this.formRef.current.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 ? (
Copy link
Contributor

Choose a reason for hiding this comment

The 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;

Copy link
Contributor

Choose a reason for hiding this comment

The 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>
);
}
Expand Down
9 changes: 6 additions & 3 deletions src/components/ScrollViewWithContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class ScrollViewWithContext extends React.Component {
this.state = {
contentOffsetY: 0,
};
this.scrollViewRef = React.createRef(null);
this.scrollViewRef = this.props.innerRef || React.createRef(null);

this.setContextScrollPosition = this.setContextScrollPosition.bind(this);
}
Expand All @@ -42,7 +42,6 @@ class ScrollViewWithContext extends React.Component {
ref={this.scrollViewRef}
onScroll={this.setContextScrollPosition}
scrollEventThrottle={this.props.scrollEventThrottle || MIN_SMOOTH_SCROLL_EVENT_THROTTLE}
scrollToOverflowEnabled
>
<ScrollContext.Provider
value={{
Expand All @@ -58,7 +57,11 @@ class ScrollViewWithContext extends React.Component {
}
ScrollViewWithContext.propTypes = propTypes;

export default ScrollViewWithContext;
export default React.forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
<ScrollViewWithContext {...props} innerRef={ref} />
));

export {
ScrollContext,
};
1 change: 1 addition & 0 deletions src/pages/AddPersonalBankAccountPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class AddPersonalBankAccountPage extends React.Component {
formID={ONYXKEYS.PERSONAL_BANK_ACCOUNT}
isSubmitButtonVisible={Boolean(this.state.selectedPlaidAccountID)}
submitButtonText={this.props.translate('common.saveAndContinue')}
scrollContextEnabled
onSubmit={this.submit}
validate={this.validate}
style={[styles.mh5, styles.flex1]}
Expand Down
2 changes: 2 additions & 0 deletions src/pages/EnablePayments/AdditionalDetailsStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ class AdditionalDetailsStep extends React.Component {
formID={ONYXKEYS.WALLET_ADDITIONAL_DETAILS}
validate={this.validate}
onSubmit={this.activateWallet}
scrollContextEnabled
scrollToOverflowEnabled
submitButtonText={this.props.translate('common.saveAndContinue')}
style={[styles.mh5, styles.flexGrow1]}
>
Expand Down
1 change: 1 addition & 0 deletions src/pages/ReimbursementAccount/BankAccountPlaidStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class BankAccountPlaidStep extends React.Component {
formID={ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM}
validate={() => ({})}
onSubmit={this.submit}
scrollContextEnabled
submitButtonText={this.props.translate('common.saveAndContinue')}
style={[styles.mh5, styles.flexGrow1]}
isSubmitButtonVisible={Boolean(selectedPlaidAccountID) && !_.isEmpty(lodashGet(this.props.plaidData, 'bankAccounts'))}
Expand Down
2 changes: 2 additions & 0 deletions src/pages/ReimbursementAccount/CompanyStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ class CompanyStep extends React.Component {
formID={ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM}
validate={this.validate}
onSubmit={this.submit}
scrollContextEnabled
scrollToOverflowEnabled
submitButtonText={this.props.translate('common.saveAndContinue')}
style={[styles.ph5, styles.flexGrow1]}
>
Expand Down
1 change: 1 addition & 0 deletions src/pages/ReimbursementAccount/RequestorStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class RequestorStep extends React.Component {
formID={ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM}
submitButtonText={this.props.translate('common.saveAndContinue')}
validate={this.validate}
scrollContextEnabled
onSubmit={this.submit}
style={[styles.mh5, styles.flexGrow1]}
>
Expand Down
1 change: 1 addition & 0 deletions src/pages/ReportSettingsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ class ReportSettingsPage extends Component {
style={[styles.mh5, styles.mt5, styles.flexGrow1]}
validate={this.validate}
onSubmit={this.updatePolicyRoomName}
scrollContextEnabled
isSubmitButtonVisible={shouldShowRoomName && !shouldDisableRename}
enabledWhenOffline
>
Expand Down
2 changes: 2 additions & 0 deletions src/pages/settings/Payments/AddDebitCardPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ class DebitCardPage extends Component {
validate={this.validate}
onSubmit={PaymentMethods.addPaymentCard}
submitButtonText={this.props.translate('common.save')}
scrollContextEnabled
scrollToOverflowEnabled
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
style={[styles.mh5, styles.flexGrow1]}
>
<TextInput
Expand Down
1 change: 1 addition & 0 deletions src/pages/workspace/WorkspaceNewRoomPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ class WorkspaceNewRoomPage extends React.Component {
<Form
formID={ONYXKEYS.FORMS.NEW_ROOM_FORM}
submitButtonText={this.props.translate('newRoomPage.createRoom')}
scrollContextEnabled
style={[styles.mh5, styles.mt5, styles.flexGrow1]}
validate={this.validate}
onSubmit={this.submit}
Expand Down
1 change: 1 addition & 0 deletions src/pages/workspace/WorkspaceSettingsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ class WorkspaceSettingsPage extends React.Component {
formID={ONYXKEYS.FORMS.WORKSPACE_SETTINGS_FORM}
submitButtonText={this.props.translate('workspace.editor.save')}
style={[styles.mh5, styles.flexGrow1]}
scrollContextEnabled
validate={this.validate}
onSubmit={this.submit}
enabledWhenOffline
Expand Down