-
Notifications
You must be signed in to change notification settings - Fork 83
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
LF-4618 Fix remove error entry when click X on number input #3643
base: integration
Are you sure you want to change the base?
Conversation
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.
Hi @Tbrid I think the defaultValue was placed as the last option was for the case where one arrives at the form with pre-filled values.
So one example is when you navigate to the next page of the form and then you go back in the form flow:
Screen.Recording.2025-01-16.at.7.19.10.PM.mov
One other thing about this field surprises me: it does not validate when blurred. It validates when another field is validated which makes sense. But it seems like it should also be validating on blur.
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.
Thanks looking good to me !
@@ -86,7 +86,7 @@ export default function NumberInput<T extends FieldValues>({ | |||
{...inputProps} | |||
className={className} | |||
error={fieldState.error?.message} | |||
onResetIconClick={reset} | |||
onResetIconClick={() => reset(defaultValue)} |
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.
initialValue
is field.value || get(formState.defaultValues, name) || defaultValue
, so I don't think this would work unless defaultValue
is passed to this component.
Can we think of a way to use the correct initialValue
instead?
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.
I am not sure what you mean? The defaultValue
is being passed as a prop.
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.
Sorry, the issue is that initialValue
is set as field.value || get(formState.defaultValues, name) || defaultValue
(line 64), but reset()
sets the value to defaultValue
.
LiteFarm/packages/webapp/src/components/Form/NumberInput/index.tsx
Lines 63 to 65 in 0f8a278
const { inputProps, reset, numericValue, increment, decrement } = useNumberInput({ | |
initialValue: field.value || get(formState.defaultValues, name) || defaultValue, | |
allowDecimal, |
When resetting, we expect the input to display initialValue
, but if field.value
or get(formState.defaultValues, name)
is used, reset()
won’t work as expected.
reset: () => update(initialValue ?? NaN)
in useNumberInput
seems correct, but the problem is that initialValue
changes. Can you think of a way to ensure reset()
always works when field.value
or get(formState.defaultValues, name)
is used as initialValue
?
Description
Bug: error value in animals count input not restored to default value when clicking "X".
Cause: default value not being used and logic of restoring default is wrong.
Fix: Change default value logic to be using prop default value instead of current value in the field and add default value prop to field input
Jira link: https://lite-farm.atlassian.net/browse/LF-4618
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: