-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add initial implementation #1
Conversation
.babelrc.js
Outdated
plugins: [ | ||
['module-resolver', { | ||
alias: { | ||
'package.json': './package.json', |
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.
Check if you will need this.
src/context/field-actions-context.js
Outdated
*/ | ||
|
||
type FieldActionsContextType = { | ||
blurField: (field: string) => void, |
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.
WDYT of string => void
or if you really want to be descriptive, (fieldName: string) => void
.
src/hooks/use-field.js
Outdated
}, [field, focusField]); | ||
|
||
return { | ||
error: get(errors, field, null), |
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.
According to useFormState type, the errors
, meta
and values
always exist, so what do you think of abstaining from using lodash
and replacing those get
's with optional chaining.
src/hooks/use-form.js
Outdated
return { | ||
...state, | ||
[payload.field]: { | ||
...get(state, [payload.field]), |
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.
...get(state, payload.field),
, also, IMO we could try to abstain from using lodash
.
result.current.onBlur(); | ||
|
||
expect(actions.blurField).toHaveBeenCalledTimes(1); | ||
expect(actions.blurField).toHaveBeenCalledWith('foo'); |
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.
We could also have tests for these same flows for the field meta
state, checking the active
and the touched
values.
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'm testing that in the useForm
tests.
src/utils/validate.js
Outdated
|
||
function getErrorRule(error) { | ||
switch (error.keyword) { | ||
case 'additionalProperties': |
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.
Why the need to rename this error type?
IMO it can still be an additionalProperties
error type.
This way we don't change the ajv
API errors.
src/hooks/use-form.js
Outdated
}, [initialValues]); | ||
|
||
const submit = useCallback((event: ?SyntheticInputEvent<any>) => { | ||
if (event && event.preventDefault) { |
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.
According to SyntheticInputEvent type, if it exists it will always have a preventDefault
method. Validating that preventDefault
exists, doesn't validate that it's a function, invoking it will throw an error.
3366c70
to
cf6ff75
Compare
@@ -0,0 +1,26 @@ | |||
/** |
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.
Add empty line before.
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.
Done.
@@ -0,0 +1,26 @@ | |||
/** |
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.
Add empty line before.
src/hooks/use-form.js
Outdated
REGISTER_FIELD: 'REGISTER_FIELD', | ||
RESET: 'RESET', | ||
SET_FIELD_VALUE: 'SET_FIELD_VALUE', | ||
SUBMIT_END: 'SUBMIT_END', |
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.
Rename this SUBMIT_END
to SUBMIT_SUCCESS
.
src/hooks/use-form.js
Outdated
} | ||
|
||
return result; | ||
}); |
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.
Shouldn't this have a cath
?
Also, couldn't finally
dispatch the actionTypes.SUBMIT_END
?
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 added the rejection callback to the then to handle onSubmit
errors.
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 also had to remove the cancellation logic, because the first dispatch in the effect was making the effect re-run and cancel the request.
cf6ff75
to
ef6d51c
Compare
ef6d51c
to
973dbb9
Compare
This PR adds the initial implementation which includes:
FormProvider
component;useForm
hook;useFormActions
hook;useFormState
hook;useField
hook;useFieldActions
hook.