-
-
Notifications
You must be signed in to change notification settings - Fork 441
feat(core): form listeners #1261
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 44a8474.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1261 +/- ##
==========================================
+ Coverage 88.83% 88.86% +0.03%
==========================================
Files 31 31
Lines 1379 1383 +4
Branches 347 347
==========================================
+ Hits 1225 1229 +4
Misses 137 137
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
67f962d
to
42e0bc0
Compare
packages/form-core/src/FieldApi.ts
Outdated
|
||
this.form.options.listeners?.onBlur?.({ | ||
fieldName: this.name, | ||
formApi: this.form, | ||
}) |
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.
Here I used to have fieldValue also being returned, however since it could be any of the field values I couldn’t type it.
I though it best to remove it and let the user access it with something like formApi.values[fieldName] so either its any, or if its a defined value such as formApi.values.age it can be inferred.
What do you think?
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.
Challenge: why don't we pass the entire fieldApi
to the callback?
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.
Well, I envisioned form.listeners being used for logging so I think I was immediately drawn to the whole state of the form. But, now that you mention it I think that also works pretty well. I would like to keep the field name and formApi immediately available for the onChange and onBlur for convenience purposes... Also, having formApi there keeps it more consistent with the onMount and onSubmission.
So something like:
this.form.options.listeners?.onBlur?.({
fieldName: this.name,
formApi: this.form,
filedApi: this
})
What do you think? This isn't a hill I want to die on though, so if your strongly against it then I'm also cool with changing it.
[edit]
So after playing around with this it'll have to be AnyFieldApi since it comes from the field instantiation and at a form level we don’t know what child it comes from so typing it won't work. But, its an easy change.
Unless you can suggest something? 😊
Note to self: move onChange out of the form listeners debounce logic and implement it in form [edit] resolved |
4005416
to
44a8474
Compare
Adds form listeners to formApi.