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

fix: should run validation on submit and clear error if a valid value is entered #520

Merged
merged 10 commits into from
Dec 3, 2023

Conversation

vikaskumar89
Copy link
Contributor

@vikaskumar89 vikaskumar89 commented Nov 10, 2023

This PR fixes issue #490 and #486.

What is changed

  • run onChange/onBlur validation on form submit . If the validation fails, form is disabled
  • clear the onSubmit error If a valid value is entered.

PR-form-1

Copy link

codesandbox-ci bot commented Nov 10, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@vikaskumar89
Copy link
Contributor Author

Hey @crutchcorn,

Could you review this please.

@danteissaias
Copy link

Is there anything currently preventing this from being merged?

@vikaskumar89
Copy link
Contributor Author

Is there anything currently preventing this from being merged?

Maintainer is very slow. cc. @tannerlinsley

@eastack
Copy link

eastack commented Dec 3, 2023

I'm not sure when the maintainer will merge this, but is there any other way we can work around these issues temporarily?

@crutchcorn
Copy link
Member

Hey all, maintainer here.

@vikaskumar89 I will need you to please start being patient with me. On top of TanStack Form not being 1.x yet (in which case bug fixes will be merged much faster), I also work on various other OSS projects, am trying to finish writing a book, am oftentimes busy with personal issues, and to top it off am not really getting paid for this work (minus a few generous donors albeit for relatively little amounts).

Please do not tag Tanner directly, and know that I will eventually come back to things here.

There is a reason I am "slow" :)

That all said, I will be reviewing this shortly.

cause === 'submit'
? onChange
? onChange
: onBlur
Copy link
Member

Choose a reason for hiding this comment

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

This is not the correct backup to cause === "submit" && !onChange

Instead, we need to run both onChange and onBlur.

I'll take over this PR to make the appropriate changes.

Copy link
Member

@crutchcorn crutchcorn left a comment

Choose a reason for hiding this comment

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

Issues with implementation have now been resolved.

In addition, I've added <Field onSubmit={z.string().min(1)}/> functionality, which was missing previously.

I also added more tests than before.

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (c2f9957) 84.55% compared to head (ec984ad) 91.31%.
Report is 54 commits behind head on main.

Files Patch % Lines
packages/form-core/src/FormApi.ts 89.55% 12 Missing and 2 partials ⚠️
packages/form-core/src/FieldApi.ts 92.85% 5 Missing ⚠️
packages/react-form/src/useField.tsx 71.42% 2 Missing ⚠️
packages/solid-form/src/createField.tsx 96.55% 1 Missing ⚠️
packages/solid-form/src/createForm.tsx 92.30% 1 Missing ⚠️
packages/solid-form/src/formContext.ts 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #520      +/-   ##
==========================================
+ Coverage   84.55%   91.31%   +6.75%     
==========================================
  Files           9       21      +12     
  Lines         395      656     +261     
  Branches      109      179      +70     
==========================================
+ Hits          334      599     +265     
+ Misses         52       50       -2     
+ Partials        9        7       -2     

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

@crutchcorn crutchcorn merged commit 344743b into TanStack:main Dec 3, 2023
5 checks passed
@vikaskumar89
Copy link
Contributor Author

vikaskumar89 commented Dec 3, 2023

Hey all, maintainer here.

@vikaskumar89 I will need you to please start being patient with me. On top of TanStack Form not being 1.x yet (in which case bug fixes will be merged much faster), I also work on various other OSS projects, am trying to finish writing a book, am oftentimes busy with personal issues, and to top it off am not really getting paid for this work (minus a few generous donors albeit for relatively little amounts).

Please do not tag Tanner directly, and know that I will eventually come back to things here.

There is a reason I am "slow" :)

That all said, I will be reviewing this shortly.

I was just hoping to get you attention to this PR. There was no other intent. 😄

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.

5 participants