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 DatePicker not saving drafts #32815

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
55 changes: 33 additions & 22 deletions src/components/DatePicker/index.js
Copy link
Contributor

Choose a reason for hiding this comment

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

DatePicker needs to be wrapped with React.forwardRef and forward the ref to the inner text input. This is needed since the Form uses that ref to focus and scroll to that element.

Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
import {setYear} from 'date-fns';
import _ from 'lodash';
import PropTypes from 'prop-types';
import React, {useEffect, useState} from 'react';
import React, {forwardRef, useState} from 'react';
import {View} from 'react-native';
import InputWrapper from '@components/Form/InputWrapper';
import * as Expensicons from '@components/Icon/Expensicons';
import refPropTypes from '@components/refPropTypes';
import TextInput from '@components/TextInput';
import {propTypes as baseTextInputPropTypes, defaultProps as defaultBaseTextInputPropTypes} from '@components/TextInput/BaseTextInput/baseTextInputPropTypes';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import CONST from '@src/CONST';
import CalendarPicker from './CalendarPicker';

const propTypes = {
/** React ref being forwarded to the DatePicker input */
forwardedRef: refPropTypes,

/**
* The datepicker supports any value that `new Date()` can parse.
* `onInputChange` would always be called with a Date (or null)
Expand All @@ -33,7 +36,12 @@ const propTypes = {
/** A maximum date of calendar to select */
maxDate: PropTypes.objectOf(Date),

...withLocalizePropTypes,
/** A function that is passed by FormWrapper */
onInputChange: PropTypes.func.isRequired,

/** A function that is passed by FormWrapper */
onTouched: PropTypes.func.isRequired,

...baseTextInputPropTypes,
};

Expand All @@ -44,40 +52,33 @@ const datePickerDefaultProps = {
value: undefined,
};

function DatePicker({containerStyles, defaultValue, disabled, errorText, inputID, isSmallScreenWidth, label, maxDate, minDate, onInputChange, onTouched, placeholder, translate, value}) {
function DatePicker({forwardedRef, containerStyles, defaultValue, disabled, errorText, inputID, isSmallScreenWidth, label, maxDate, minDate, onInputChange, onTouched, placeholder, value}) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const [selectedDate, setSelectedDate] = useState(value || defaultValue || undefined);

useEffect(() => {
if (selectedDate === value || _.isUndefined(value)) {
return;
}
setSelectedDate(value);
}, [selectedDate, value]);
Comment on lines -51 to -56
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this useEffect makes the value prop behaves just like defaultValue thus making the DatePicker a non-controllable component (you can't change it's value programmatically). This does not seem to be a problem now so it may not be a blocker but I think we need to at least make it clear that the value prop is only there because it's needed by the FormProvider (and it will not control the datepicker actual value)

cc @luacmartins thoughts on the above ^

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems fine to me


useEffect(() => {
const onSelected = (newValue) => {
if (_.isFunction(onTouched)) {
onTouched();
}
if (_.isFunction(onInputChange)) {
onInputChange(selectedDate);
onInputChange(newValue);
}
// To keep behavior from class component state update callback, we want to run effect only when the selected date is changed.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectedDate]);
setSelectedDate(newValue);
};
Comment on lines -58 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason on why we are calling onInputChange too early? Calling it in the useEffect (as it was) looks better since it's a side effect.

    useEffect(() => {
        if (_.isFunction(onTouched)) {
            onTouched();
        }
        if (_.isFunction(onInputChange)) {
            onInputChange(selectedDate);
        }
    }, [selectedDate]);
                    onSelected={setSelectedDate}

Copy link
Contributor Author

@kowczarz kowczarz Dec 14, 2023

Choose a reason for hiding this comment

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

I disagree that it's called too early. Currently its behaviour is similar to onValueChange from FormProvider - it's a callback, which is fired when we are certain, that the state will be changed and we know the new state. Additionally it seems like event-specific logic inside effect, and that's a React anti-patern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!


return (
<View style={styles.datePickerRoot}>
<View style={[isSmallScreenWidth ? styles.flex2 : {}, styles.pointerEventsNone]}>
<InputWrapper
InputComponent={TextInput}
<TextInput
ref={forwardedRef}
inputID={inputID}
forceActiveLabel
icon={Expensicons.Calendar}
label={label}
accessibilityLabel={label}
role={CONST.ROLE.PRESENTATION}
value={value || selectedDate || ''}
value={selectedDate}
placeholder={placeholder || translate('common.dateFormat')}
errorText={errorText}
containerStyles={containerStyles}
Expand All @@ -92,7 +93,7 @@ function DatePicker({containerStyles, defaultValue, disabled, errorText, inputID
minDate={minDate}
maxDate={maxDate}
value={selectedDate}
onSelected={setSelectedDate}
onSelected={onSelected}
/>
</View>
</View>
Expand All @@ -103,4 +104,14 @@ DatePicker.propTypes = propTypes;
DatePicker.defaultProps = datePickerDefaultProps;
DatePicker.displayName = 'DatePicker';

export default withLocalize(DatePicker);
const DatePickerWithRef = forwardRef((props, ref) => (
<DatePicker
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
forwardedRef={ref}
/>
));

DatePickerWithRef.displayName = 'DatePickerWithRef';

export default DatePickerWithRef;
4 changes: 3 additions & 1 deletion src/pages/EditRequestCreatedPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import PropTypes from 'prop-types';
import React from 'react';
import DatePicker from '@components/DatePicker';
import FormProvider from '@components/Form/FormProvider';
import InputWrapper from '@components/Form/InputWrapper';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
import useLocalize from '@hooks/useLocalize';
Expand Down Expand Up @@ -34,7 +35,8 @@ function EditRequestCreatedPage({defaultCreated, onSubmit}) {
submitButtonText={translate('common.save')}
enabledWhenOffline
>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="created"
label={translate('common.date')}
defaultValue={defaultCreated}
Expand Down
3 changes: 2 additions & 1 deletion src/pages/EnablePayments/AdditionalDetailsStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,8 @@ function AdditionalDetailsStep({walletAdditionalDetails, translate, currentUserP
placeholder={translate('common.phoneNumberPlaceholder')}
shouldSaveDraft
/>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="dob"
containerStyles={[styles.mt4]}
label={translate(fieldNameTranslationKeys.dob)}
Expand Down
8 changes: 6 additions & 2 deletions src/pages/ReimbursementAccount/CompanyStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,18 @@ function CompanyStep({reimbursementAccount, reimbursementAccountDraft, getDefaul
InputComponent={Picker}
inputID="incorporationType"
label={translate('companyStep.companyType')}
items={_.map(_.keys(CONST.INCORPORATION_TYPES), (key) => ({value: key, label: translate(`companyStep.incorporationTypes.${key}`)}))}
items={_.map(_.keys(CONST.INCORPORATION_TYPES), (key) => ({
value: key,
label: translate(`companyStep.incorporationTypes.${key}`),
}))}
placeholder={{value: '', label: '-'}}
defaultValue={getDefaultStateForField('incorporationType')}
shouldSaveDraft
/>
</View>
<View style={styles.mt4}>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="incorporationDate"
label={translate('companyStep.incorporationDate')}
placeholder={translate('companyStep.incorporationDatePlaceholder')}
Expand Down
12 changes: 2 additions & 10 deletions src/pages/ReimbursementAccount/IdentityForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ const propTypes = {
/** Style for wrapping View */
style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

/** Callback fired when a field changes. Passes args as {[fieldName]: val} */
onFieldChange: PropTypes.func,

/** Form values */
values: PropTypes.shape({
/** First name field */
Expand Down Expand Up @@ -127,7 +124,6 @@ const defaultProps = {
ssnLast4: '',
},
shouldSaveDraft: false,
onFieldChange: () => {},
};

function IdentityForm(props) {
Expand All @@ -152,7 +148,6 @@ function IdentityForm(props) {
role={CONST.ROLE.PRESENTATION}
value={props.values.firstName}
defaultValue={props.defaultValues.firstName}
onChangeText={(value) => props.onFieldChange({firstName: value})}
errorText={props.errors.firstName ? props.translate('bankAccount.error.firstName') : ''}
/>
</View>
Expand All @@ -166,19 +161,18 @@ function IdentityForm(props) {
role={CONST.ROLE.PRESENTATION}
value={props.values.lastName}
defaultValue={props.defaultValues.lastName}
onChangeText={(value) => props.onFieldChange({lastName: value})}
errorText={props.errors.lastName ? props.translate('bankAccount.error.lastName') : ''}
/>
</View>
</View>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID={props.inputKeys.dob}
shouldSaveDraft={props.shouldSaveDraft}
label={`${props.translate('common.dob')}`}
containerStyles={[styles.mt4]}
placeholder={props.translate('common.dateFormat')}
defaultValue={props.values.dob || props.defaultValues.dob}
onInputChange={(value) => props.onFieldChange({dob: value})}
Copy link
Contributor

Choose a reason for hiding this comment

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

onInputChange is being overwritten by the FormProvider. It's indeed useless to pass it here. But we still need to call onFieldChange. We can use the onValueChange callback for that.

                onValueChange={(value) => props.onFieldChange({dob: value})}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and both props.onFieldChange and props.values from IdentityForm aren't used anywhere. So I would get rid of them completely. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's sounds good. Let's remove onFieldChange from AddressForm as well. But we may keep values for now (just feel like it should be there since defaultValues is there - no strong preference anyway)

errorText={dobErrorText}
minDate={minDate}
maxDate={maxDate}
Expand All @@ -193,7 +187,6 @@ function IdentityForm(props) {
containerStyles={[styles.mt4]}
inputMode={CONST.INPUT_MODE.NUMERIC}
defaultValue={props.defaultValues.ssnLast4}
onChangeText={(value) => props.onFieldChange({ssnLast4: value})}
errorText={props.errors.ssnLast4 ? props.translate('bankAccount.error.ssnLast4') : ''}
maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.SSN}
/>
Expand All @@ -205,7 +198,6 @@ function IdentityForm(props) {
values={_.omit(props.values, identityFormInputKeys)}
defaultValues={_.omit(props.defaultValues, identityFormInputKeys)}
errors={props.errors}
onFieldChange={props.onFieldChange}
/>
</View>
);
Expand Down
4 changes: 3 additions & 1 deletion src/pages/iou/MoneyRequestDatePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import DatePicker from '@components/DatePicker';
import FormProvider from '@components/Form/FormProvider';
import InputWrapper from '@components/Form/InputWrapper';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
import useLocalize from '@hooks/useLocalize';
Expand Down Expand Up @@ -99,7 +100,8 @@ function MoneyRequestDatePage({iou, route, selectedTab}) {
submitButtonText={translate('common.save')}
enabledWhenOffline
>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="moneyRequestCreated"
label={translate('common.date')}
defaultValue={iou.created}
Expand Down
4 changes: 3 additions & 1 deletion src/pages/iou/request/step/IOURequestStepDate.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import dateSubtract from 'date-fns/sub';
import React from 'react';
import DatePicker from '@components/DatePicker';
import FormProvider from '@components/Form/FormProvider';
import InputWrapper from '@components/Form/InputWrapper';
import transactionPropTypes from '@components/transactionPropTypes';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
Expand Down Expand Up @@ -67,7 +68,8 @@ function IOURequestStepDate({
submitButtonText={translate('common.save')}
enabledWhenOffline
>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="moneyRequestCreated"
label={translate('common.date')}
defaultValue={transaction.created}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import React, {useCallback} from 'react';
import {withOnyx} from 'react-native-onyx';
import DatePicker from '@components/DatePicker';
import FormProvider from '@components/Form/FormProvider';
import InputWrapper from '@components/Form/InputWrapper';
import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
Expand Down Expand Up @@ -81,7 +82,8 @@ function DateOfBirthPage({translate, privatePersonalDetails}) {
submitButtonText={translate('common.save')}
enabledWhenOffline
>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="dob"
label={translate('common.date')}
defaultValue={privatePersonalDetails.dob || ''}
Expand Down
3 changes: 2 additions & 1 deletion src/stories/Form.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ function Template(args) {
containerStyles={[defaultStyles.mt4]}
hint="No PO box"
/>
<DatePicker
<InputWrapper
InputComponent={DatePicker}
inputID="dob"
label="Date of Birth"
containerStyles={[defaultStyles.mt4]}
Expand Down
Loading