Fix DatePicker not saving drafts #32815
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
| 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.
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})}There was a problem hiding this comment.
I checked and both props.onFieldChange and props.values from IdentityForm aren't used anywhere. So I would get rid of them completely. WDYT?
There was a problem hiding this comment.
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)
| useEffect(() => { | ||
| if (selectedDate === value || _.isUndefined(value)) { | ||
| return; | ||
| } | ||
| setSelectedDate(value); | ||
| }, [selectedDate, value]); |
There was a problem hiding this comment.
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 ^
| 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); | ||
| }; |
There was a problem hiding this comment.
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}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chrome
iOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safari
MacOS: Desktop
|
|
@kowczarz Please add a test case to verify that the draft is being saved correctly |
…ate-picker-not-saving-drafts # Conflicts: # src/components/DatePicker/index.js
src/components/DatePicker/index.js
Outdated
| const DatePickerWithRef = forwardRef((props, ref) => ( | ||
| <DatePickerWithRef | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} | ||
| forwardedRef={ref} | ||
| /> | ||
| )); | ||
|
|
||
| DatePickerWithRef.displayName = 'DatePickerWithRef'; |
There was a problem hiding this comment.
We can just use forwardRef directly on the DataPicker. No need to pass forwardedRefas a prop.
function DatePicker({containerStyles, defaultValue, disabled, errorText, inputID, isSmallScreenWidth, label, maxDate, minDate, onInputChange, onTouched, placeholder, value}, ref) { ... }
export default React.forwardRef(DatePicker)There was a problem hiding this comment.
Good point, we can consider refactoring other places where it's done that way.
There was a problem hiding this comment.
Actually we can't do it yet, because forwardRef render functions do not support propTypes or defaultProps. so we will be able to do it after TS migration.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually you are right. Ignore my above comment.
|
I see one more issue with ref, but I will solve it tomorrow. |
|
I think it's ready for the final review. |
|
@kowczarz What about #32815 (comment)? |
|
@s77rt I had to missed it. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.14-0 🚀
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.14-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀
|
Details
Fixed Issues
$ #31612
Tests
Check all screens, that contain DateInputs
Check error flow
Drafts saving flow:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop