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

Add isValidating to field state #10657

Merged

Conversation

pbankonier
Copy link
Contributor

@pbankonier pbankonier commented Jul 10, 2023

close #8160

This PR includes the following:

  • add validatingFields property to formState
  • add isValidating to fieldState

Motivation behind this is that we want to display an indicator at individual form fields when they are validating.

There was already an issue where some people requested that feature a while ago: #5744

I also already created a branch in my fork of the documentation for this feature (react-hook-form/documentation@master...pbankonier:react-hook-form-documentation:feature/fieldState_isValidating)

@codesandbox
Copy link

codesandbox bot commented Jul 10, 2023

This branch is running in CodeSandbox. Use the links below to review this PR faster.


CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders | Preview

@pbankonier
Copy link
Contributor Author

Hey @bluebill1049,
since I opened this PR a few weeks ago, I just wanted to ask if you had time to take a look at it and if so, if you have any feedback on this.

@pbankonier
Copy link
Contributor Author

I did some adjustments to the formState validatingFields property (it uses a boolean instead of a counter now). The use of a counter here was wrong from the beginning I guess, because stapled validators shouldn't be possible. This was fixed with #10949.
@bluebill1049 Just wanted to check back if you have found the time to take a look at this and what your thoughts are regarding this feature? Thanks in advance.

@NicoWohlfarth
Copy link

@bluebill1049 Any chance on getting a review on this?

Copy link

stale bot commented Dec 15, 2023

Thank you for your contributions! This Pull Request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Best, RHF Team ❤️

@stale stale bot added the stale label Dec 15, 2023
@NicoWohlfarth
Copy link

Hey @bluebill1049, I'm kindly asking again if there is anything we can do to make a review on this happen?

@stale stale bot removed the stale label Dec 20, 2023
@primus852
Copy link

Hey,
this looks like a great improvement we could use in our project. Is there anything we can do to move this forward?

Best,

Copy link
Member

@bluebill1049 bluebill1049 left a comment

Choose a reason for hiding this comment

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

Awesome stuff! sorry for the late review, I have been very busy with everything.

Comment on lines 1718 to 1719
let formState: any;
let getFieldState: any;
Copy link
Member

Choose a reason for hiding this comment

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

could we please remove any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 1809 to 1810
let formState: any;
let getFieldState: any;
Copy link
Member

Choose a reason for hiding this comment

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

same here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 1885 to 1886
let getFieldState: any;
let formState: any;
Copy link
Member

Choose a reason for hiding this comment

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

here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 1961 to 1962
let unregister: any;
let formState: any;
Copy link
Member

Choose a reason for hiding this comment

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

👁️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 181 to 187
const isAnyFieldValidating = Object.values(
_formState.validatingFields,
).some((val) => val);
_proxyFormState.isValidating &&
_subjects.state.next({
isValidating: isAnyFieldValidating,
});
Copy link
Member

Choose a reason for hiding this comment

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

i think this can be wrapped within the _proxyFormState check.

Comment on lines 181 to 183
const isAnyFieldValidating = Object.values(
_formState.validatingFields,
).some((val) => val);
Copy link
Member

Choose a reason for hiding this comment

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

utils/isEmptyObject.ts check will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isEmptyObject is not doing what I want here. I extracted this to a util method called objectHasTruthyValue.

Comment on lines 191 to 194
set(_formState.validatingFields, name, isValidating);
_subjects.state.next({
isValidating: value,
validatingFields: _formState.validatingFields,
});
Copy link
Member

Choose a reason for hiding this comment

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

can move to the _updateIsValidating function and wrapped within the proxy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored updateIsValidating and updateValidatingFields into one method and added the check for the proxyFormState.

Comment on lines 819 to 821
fieldNames.forEach((name) => {
_updateValidatingFields(true, name);
});
Copy link
Member

Choose a reason for hiding this comment

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

can we do that within the _updateValidatingFields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this so that _updateValidatingFields has an array of field names as its argument.

@bluebill1049
Copy link
Member

Nice thanks for the update! I will take a look at it again this weekend. 👍

Copy link
Member

@bluebill1049 bluebill1049 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for following the code style as well. It's not the most readable but definitely save bytes for the output bundle. We can roll this out as the next feature release. Great stuff.

@bluebill1049 bluebill1049 merged commit ebc1f2c into react-hook-form:master Feb 24, 2024
7 checks passed
@bluebill1049
Copy link
Member

Todo list

  • update doc

rafaelcalhau pushed a commit to rafaelcalhau/react-hook-form that referenced this pull request May 5, 2024
…k-form#10657)

* Add isValidating to field state

* Fix type tests for isValidating field state

* Adjust bundlewatch maxSize

* Fix api-extractor.md merge fail

* Adjust formState validatingFields property

* Adjust bundlewatch maxSize

* Refactoring for isValidating fieldState

* Fix multi async validators behavior

* Remove unnecessary explicit type definition in isValidating test

* Improve _updateIsValidating method

* Add proxyFormState check to updateIsValidating method

---------

Co-authored-by: Beier (Bill) <bluebill1049@hotmail.com>
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.

better support for async validation and granular subscribable "isValidating" state on the field level
5 participants