Skip to content

Fix removing validation + Undebounce - #110

Merged
haacked merged 18 commits into
mainfrom
gh100-fix-remove
May 6, 2024
Merged

Fix removing validation + Undebounce#110
haacked merged 18 commits into
mainfrom
gh100-fix-remove

Conversation

@dahlbyk

@dahlbyk dahlbyk commented Apr 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

More than I usually like to change at once, but all interrelated. In particular:

  • Fixes some bugs related to removing validation from a field/form
  • Removing validation from a field/form now also unwires event handlers
  • Validation is now only debounced on input/change; manual validation calls start (and resolve, if none are async) immediately
  • validateForm() and validateField() have been changed from returning void to returning Promise<boolean>, resolving with the same value their callback would receive.

@dahlbyk
dahlbyk requested a review from haacked April 26, 2024 15:12
This was referenced Apr 26, 2024
Closed
Closed
@haacked

haacked commented May 2, 2024

Copy link
Copy Markdown
Owner

I've started reviewing this.

haacked
haacked requested changes May 2, 2024
View reviewed changes

@haacked haacked left a comment

Copy link
Copy Markdown
Owner

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.

Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
private untrackFormInput(form: HTMLFormElement, inputUID: string) {
let formUID = this.getElementUID(form);
if (!this.formInputs[formUID]) {
let formInputs = this.formInputs[formUID]

@haacked haacked May 2, 2024

Copy link
Copy Markdown
Owner

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?

@dahlbyk dahlbyk May 4, 2024

Copy link
Copy Markdown
Collaborator Author

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.

dahlbyk added 13 commits May 4, 2024 16:58
@dahlbyk
Remove form event listeners
1938d05
@dahlbyk
Refactor private scans to use callback
f083dcf
@dahlbyk
Allow scan/remove/watch without root
c83f25a
@dahlbyk
Store messages per form
7edae2d
@dahlbyk
Untrack form messages
d8fa06e
@dahlbyk
Remove validatable assumption
d51a62c
@dahlbyk
Simplify resetField; clear message class, too
385a3cf
@dahlbyk
Only debounce validation on input event
5b7d89b 
Manual validation is no longer debounced:
- validateField()
- isFieldValid()
- validateForm()
@dahlbyk
Capture validation task
ac14f19
@dahlbyk
Refactor to async/await
c838a40
@dahlbyk
Return promise from validateForm() and validateField()
ab68138 
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.
@dahlbyk
Consistent usage of this.formInputs
8043c23
@dahlbyk
Build 0.10.0
2f5020d
@dahlbyk
dahlbyk force-pushed the gh100-fix-remove branch from 54019cc to 2f5020d Compare May 4, 2024 22:01
@dahlbyk

dahlbyk commented May 6, 2024

Copy link
Copy Markdown
Collaborator Author

I did push a fix for the bug Phil found, and rebuilt 0.10.0.

haacked
haacked approved these changes May 6, 2024
View reviewed changes

@haacked haacked left a comment

Copy link
Copy Markdown
Owner

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.

@haacked
haacked merged commit fd1e5a6 into main May 6, 2024
@haacked
haacked deleted the gh100-fix-remove branch May 6, 2024 19:52
@haacked haacked mentioned this pull request May 6, 2024
Open
@lonix1

lonix1 commented May 7, 2024

Copy link
Copy Markdown
Contributor

WOW! Nice work!

@lonix1

lonix1 commented May 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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:

  • I think the demo has the button text toggling the wrong way round. One must click the remove button repeatedly to show the expected text. Very minor point, not important, just something I found.

Questions, I want to know whether I understand the new behaviour:

  • To remove validation from a form, one uses v.remove(formElement)
  • One can readd validation to that form via v.scan(formElement)
  • Validation failure classes remain after removal, e.g. input-validation-error. This is by design as the form's original state could have been server-side errors. If the user wants different behaviour he can reset/remove them.
  • Is there a way to perform the "full reset" idea from this comment, or should one do that manually (possibly by clicking on the form's "reset" button, if it has one)?
  • I'm using document.querySelectorAll('form') or someButton.form to target the form; out of curiosity only (because the library has many cool undocumented features), is it possible to tell which forms the library is currently tracking? UPDATE: a reasonable way is let forms = [...new Set(Array.from(document.querySelectorAll('[data-val]')).map(x => x.form))]; forms.forEach(form => v.remove(form));

Anything else I need to know?

(Sorry for the long list of questions. The new bits are really useful, thanks!)

@haacked

haacked commented May 8, 2024

Copy link
Copy Markdown
Owner

@lonix1 Great questions! Would you mind opening a new issue with these questions? New questions on a closed PR are easy to overlook. 😄

@lonix1 lonix1 mentioned this pull request May 9, 2024
Open
@lonix1

lonix1 commented May 9, 2024

Copy link
Copy Markdown
Contributor

Sorry I thought opening a new issue would be spamming your repo. :) I've done so here: #114

@dahlbyk dahlbyk mentioned this pull request Sep 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haacked haacked haacked approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Initial isFieldValid returns true when field is empty and uses required validator. Disabling validation

3 participants

@dahlbyk @haacked @lonix1