Skip to content
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

feat(core): add field listeners #1032

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

harry-whorlow
Copy link

@harry-whorlow harry-whorlow commented Nov 22, 2024

Background

From discussion #709 and PR #801

A continuation of the PR #801 that been sitting stale since July, as per the previous pull request:

Sometimes we may want to attach our own side effects to the field when some event happens. 
For example, resetting another field if some field value has changed.

Attaching these 'listeners' inside the validator does not work as intended, because validator 
does not guarantee if it was triggered by the exact event. 
(eg. all validators run when form submits)

It's of course possible to implement, but it's not very clean and the concerns become scattered.

The PR was given the go ahead by @crutchcorn here.

This is something we would really like to have, as I'm currently experiencing a minor blocker with this functionality missing, and I would prefer to have a clear api as opposed to using the current validators onChange workaround.


Continuation of work

  • rebased to current main
  • removed onHandleChange listener & tests
  • add onBlur listener & tests
  • add onSubmit tests (need guidance on functionality)

Guidance wanted

Specifically referring to the onSubmit comment made here

Is there a method that I've missed for an "on submit" callback that I can use to set the listeners value inside the FiledApi when the form is submitted? or is this something I need to create and pass down from the form to the field? Just a hint in the correct direction would be really appreciated.

Feel free to fire off any questions, I'll do my best to get back to you in a timely manner.

Looking forward to hearing from the maintainers 🤟

Functionality Tests Docs
[x] onChange [x] onChange [ ] doc structure
[x] onBlur [x] onBlur [ ] React
[x] onMount [x] onMount [ ] Vue
[x] onSubmit [x] onSubmit [ ] Anngular

@harry-whorlow
Copy link
Author

Moved the onSubmit test from FieldApi.spec.ts -> FormApi.spec.ts

Copy link

nx-cloud bot commented Nov 22, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a0dd15c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Nov 22, 2024

Open in Stackblitz

More templates

@tanstack/angular-form

pnpm add https://pkg.pr.new/@tanstack/angular-form@1032

@tanstack/form-core

pnpm add https://pkg.pr.new/@tanstack/form-core@1032

@tanstack/react-form

pnpm add https://pkg.pr.new/@tanstack/react-form@1032

@tanstack/lit-form

pnpm add https://pkg.pr.new/@tanstack/lit-form@1032

@tanstack/solid-form

pnpm add https://pkg.pr.new/@tanstack/solid-form@1032

@tanstack/valibot-form-adapter

pnpm add https://pkg.pr.new/@tanstack/valibot-form-adapter@1032

@tanstack/vue-form

pnpm add https://pkg.pr.new/@tanstack/vue-form@1032

@tanstack/yup-form-adapter

pnpm add https://pkg.pr.new/@tanstack/yup-form-adapter@1032

@tanstack/zod-form-adapter

pnpm add https://pkg.pr.new/@tanstack/zod-form-adapter@1032

commit: a0dd15c

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.06%. Comparing base (c8e9887) to head (a0dd15c).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1032      +/-   ##
==========================================
+ Coverage   85.70%   86.06%   +0.36%     
==========================================
  Files          28       28              
  Lines        1098     1105       +7     
  Branches      275      275              
==========================================
+ Hits          941      951      +10     
+ Misses        144      141       -3     
  Partials       13       13              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@Balastrong Balastrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic PR, very well done! :D

I think you can tick the onSubmit box and go ahead with the docs.

packages/form-core/src/FieldApi.ts Outdated Show resolved Hide resolved
packages/form-core/src/FormApi.ts Outdated Show resolved Hide resolved
@harry-whorlow
Copy link
Author

harry-whorlow commented Nov 22, 2024

With regards to your comment, I would say yes unless you have a leaning in a different direction... But, I'm completely open to suggestions.

@harry-whorlow
Copy link
Author

@Balastrong hot off the press with a first draft for the docs, let me know what you think.

Once I get the thumbs up I'll create the Vue and Angular docs. Feel free to offer improvements and tweaks 🤟

packages/form-core/src/FormApi.ts Outdated Show resolved Hide resolved
packages/form-core/tests/FormApi.spec.ts Outdated Show resolved Hide resolved
packages/form-core/src/FieldApi.ts Outdated Show resolved Hide resolved
docs/framework/react/guides/listeners.md Outdated Show resolved Hide resolved
docs/framework/react/guides/listeners.md Outdated Show resolved Hide resolved
docs/framework/react/guides/basic-concepts.md Show resolved Hide resolved
@harry-whorlow
Copy link
Author

@Balastrong, let me know what you think! if everything looks good I'll finish the Vue and Angular when I get back tonight. Enjoy the weekend!🤟

@ha1fstack
Copy link

Thank you for following up the original PR 👏

Copy link
Member

@Balastrong Balastrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm looking at all the tiny details now, but everything else looks really great :)

Please go ahead with the docs for the other frameworks and I think we're good to go!

Comment on lines 206 to 209
name="country"
listener={{
onChange: ({ value }) => {
form.reset({county: ''})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name="country"
listener={{
onChange: ({ value }) => {
form.reset({county: ''})
name="country"
listener={{
onChange: ({ value }) => {
form.reset({ province: '' })

Let's also update this to use province (and spaces between { and })

docs/framework/react/guides/listeners.md Outdated Show resolved Hide resolved
@harry-whorlow
Copy link
Author

@Balastrong no please, nitpick away... I'll be the poor sod using it 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants