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

Do not validate disabled fields #94

Merged
merged 12 commits into from
Mar 19, 2024
Merged

Conversation

haacked
Copy link
Owner

@haacked haacked commented Jan 2, 2024

Fixes #90

Note this does not handle disabled fieldsets because we just rely on the browser for that. If that assumption is incorrect, we can fix that in a subsequent PR.

@haacked haacked requested a review from dahlbyk January 2, 2024 20:43
src/index.ts Outdated Show resolved Hide resolved
@haacked
Copy link
Owner Author

haacked commented Jan 5, 2024

I noticed that when a disabled element is valid, we mark it green. We should completely ignore disabled elements. I'll dig into this a bit more.

@haacked haacked force-pushed the haacked/90-ignore-disabled-fields branch from 455e75b to 98b4a3e Compare January 5, 2024 21:48
@haacked haacked force-pushed the haacked/90-ignore-disabled-fields branch from 5d4cc4f to 52446b9 Compare January 6, 2024 03:02
@haacked
Copy link
Owner Author

haacked commented Jan 6, 2024

Ok, I updated the disabled fields demo to test dynamic support for fields that are enabled/disabled. Turns out the value of disabled is not in the attributes collection nor in the oldValue property of MutationRecord, so we need to do some special casing.

Note: for this demo to work, we need to be watching for DOM changes, hence:

<script>
    const service = new aspnetValidation.ValidationService(console);
    service.bootstrap({ watch: true });
</script>

You can dynamically enable/disable the two inputs and validation works correctly. For example, by default, input 1 is disabled and input 2 is enabled, so submitting the form with no values yields:

image

If I enable input 1 and then try to submit again, we see it now shows an error message.

Screenshot 2024-01-05 at 7 00 16 PM

If I disable both, the form is valid and submits successfully.

@haacked haacked requested a review from dahlbyk January 6, 2024 03:07
Copy link
Collaborator

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

We ended up not using watch: true. I think we should try to make this work as expected without.

The demo would re-validate the input in updateStatus() to handle the reset - if a form has enabled/disable behavior I don't think that's too much to ask.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@haacked
Copy link
Owner Author

haacked commented Jan 8, 2024

We ended up not using watch: true. I think we should try to make this work as expected without.

The demo would re-validate the input in updateStatus() to handle the reset - if a form has enabled/disable behavior I don't think that's too much to ask.

I think we should have two demos. It should Just Work™️ if watch: true. Otherwise, we make the user re-validate the input if they disable/enable it.

@haacked haacked requested a review from dahlbyk January 9, 2024 00:23
@haacked
Copy link
Owner Author

haacked commented Jan 9, 2024

Ok, try that out and let me know what you think.

@haacked
Copy link
Owner Author

haacked commented Feb 6, 2024

@dahlbyk do you want to take another look or should I go ahead and merge?

Copy link
Collaborator

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

Well I apparently had some pending comments, so let's start there? 😅

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
This way, if they become enabled, we'll validate them. If they become disabled, we won't validate them.
@haacked
Copy link
Owner Author

haacked commented Mar 18, 2024

I made some changes thanks to @dahlbyk's feedback. His feedback made me realize that ignoring validation on a disabled field is orthogonal to whether we update the input's current validation state when it is enabled/disabled.

So we never validate a disabled input. We always validate an enabled input.

However, if an input is currently enabled and showing a validation error and you dynamically disable the input, the validation error will continue to be displayed. But the next time you submit the form, the field will be ignored and the validation message will be cleared.

If you want the validation message to be cleared at the time the input is disabled, there's two options:

  1. service.bootstrap({ watch: true });
  2. service.reset(input);

I've included demos of both.

`as` in TypeScript is not the same as in C#.
Copy link
Collaborator

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

I didn't kick the tires, but the changes align with what I had in mind. 👍

@haacked haacked merged commit b9b8fd2 into main Mar 19, 2024
1 check passed
@haacked haacked deleted the haacked/90-ignore-disabled-fields branch March 19, 2024 19:11
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.

Should not validate disabled fields
2 participants