-
Notifications
You must be signed in to change notification settings - Fork 25
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 removing validation + Undebounce #110
Conversation
I've started reviewing this. |
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.
Found a bug during my testing. It's in the review.
src/index.ts
Outdated
@@ -1011,12 +1023,20 @@ export class ValidationService { | |||
|
|||
private untrackFormInput(form: HTMLFormElement, inputUID: string) { | |||
let formUID = this.getElementUID(form); | |||
if (!this.formInputs[formUID]) { | |||
let formInputs = this.formInputs[formUID] |
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 think this is a little confusing (as evidenced by the bug I point out later). As I understand it, this.formInputs
is a way of looking up all form inputs for a form. And formInputs
is all the inputs for the current form. So we may want to rename this local variable something like currentFormInputs
.
And for extra credit, perhaps this.formInputs
should be this.formsInputs
or this.forms
?
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.
For now I've applied consistency in usage/naming (formInputUIDs = this.formInputs[formUID]
), which (combined with your "use the variable" suggestion) makes the bug more obvious.
A more precise rename (inputsByFormUID
?) might be useful, but there are several similar lookup fields we'd want to update for consistency.
Manual validation is no longer debounced: - validateField() - isFieldValid() - validateForm()
These have returned void, so it's not a breaking change. isFieldValid() returns boolean; changing that to Promise<boolean> would be a breaking change, so it's left as-is for now.
I did push a fix for the bug Phil found, and rebuilt 0.10.0. |
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.
Nice work!
Might be nice to include an example of how to clear existing validation messages when you disable validation. But that can happen later.
WOW! Nice work! |
I'd like to write some docs for the "remove validation" feature. (I'm unfamiliar with the history of the "undebounce" feature, so I'll leave that for another day or another person.) Comments:
Questions, I want to know whether I understand the new behaviour:
Anything else I need to know? (Sorry for the long list of questions. The new bits are really useful, thanks!) |
@lonix1 Great questions! Would you mind opening a new issue with these questions? New questions on a closed PR are easy to overlook. 😄 |
Sorry I thought opening a new issue would be spamming your repo. :) I've done so here: #114 |
More than I usually like to change at once, but all interrelated. In particular:
validateForm()
andvalidateField()
have been changed from returningvoid
to returningPromise<boolean>
, resolving with the same value theircallback
would receive.