Skip to content

Conversation

AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Jun 5, 2023

We had this issue raised on the Umbraco Forms product that uses this library for client side validation.

It seems to me a reasonable general case for input fields, particularly check box and radio button lists, that when adding or removing a class to indicate an error on an input field, that this should be applied to all inputs with the same name.

Hence this PR.

As an example, with a form containing mandatory radio button or check box lists, with .input-validation-error styled with red text:

image

Submitting the form triggers the client-side validation and the appropriate classes are added to all fields:

image

Clicking a single radio or checkbox will currently remove the error only for the one you clicked on, leaving the other indicated as invalid even though now actually it's not.

With this PR all radio buttons will have the class removed.

image

Similarly for adding errors, previously, if values in a list of checkboxes were selected and subsequently removed, when the last item was deselected, only that one would get the invalid error class. Now all of them do:

image

Copy link
Owner

@haacked haacked left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'll want to test it out first.

AndyButland and others added 2 commits June 6, 2023 06:28
I couldn't get it to work with radio buttons because I couldn't get it to show them as invalid.
Copy link
Owner

@haacked haacked left a comment

Choose a reason for hiding this comment

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

:shipit: Thanks for the submission!

Copy link
Owner

@haacked haacked left a comment

Choose a reason for hiding this comment

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

:shipit: Thanks for the submission!

@haacked haacked merged commit de72612 into haacked:main Jun 6, 2023
@haacked
Copy link
Owner

haacked commented Jun 6, 2023

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.

2 participants