-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Changes from 5 commits
c7a0593
1ffbeb3
a2bbad9
8258408
8c87511
8425d25
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,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) | ||
|
@@ -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, | ||
}; | ||
|
||
|
@@ -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
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. 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 ^ 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. 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
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. Any specific reason on why we are calling useEffect(() => {
if (_.isFunction(onTouched)) {
onTouched();
}
if (_.isFunction(onInputChange)) {
onInputChange(selectedDate);
}
}, [selectedDate]); onSelected={setSelectedDate} 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 disagree that it's called too early. Currently its behaviour is similar to 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. 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} | ||
|
@@ -92,7 +93,7 @@ function DatePicker({containerStyles, defaultValue, disabled, errorText, inputID | |
minDate={minDate} | ||
maxDate={maxDate} | ||
value={selectedDate} | ||
onSelected={setSelectedDate} | ||
onSelected={onSelected} | ||
/> | ||
</View> | ||
</View> | ||
|
@@ -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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,14 +171,14 @@ function IdentityForm(props) { | |
/> | ||
</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})} | ||
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.
onValueChange={(value) => props.onFieldChange({dob: value})} 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 checked and both 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. That's sounds good. Let's remove |
||
errorText={dobErrorText} | ||
minDate={minDate} | ||
maxDate={maxDate} | ||
|
There was a problem hiding this comment.
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.