Skip to content

Conversation

davidkarlsson
Copy link
Contributor

DOMTokenList::add() throws a DOMException if you try to add an empty string as a class. This check prevents this from happening so that you can set ValidationInputValidCssClassName etc. to an empty string if you don't want to add classes for valid input fields or messages for instance.

DOMTokenList::add() throws a DOMException if you try to add an empty string as a class. This check prevents this from happening so that you can set ValidationInputValidCssClassName etc. to an empty string if you don't want to add classes for valid input fields or messages for instance.
if (addClass && !element.classList.contains(addClass)) {
element.classList.add(addClass);
}
if (element.classList.contains(removeClass)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Does this throw if removeClass is blank? I wonder if we should protect against that 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.

Yes, it throws if you call element.classList.remove('') but the contains(removeClass) check prevents that from happening so I didn't add it for that case.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, makes sense. Thanks for following up and the contribution!

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.

I'll take this as-is. We may want to add the same check for removeClass in a future PR.

@haacked haacked merged commit 2430929 into haacked:main Nov 2, 2023
@haacked
Copy link
Owner

haacked commented Nov 2, 2023

This is now live on NPM https://www.npmjs.com/package/aspnet-client-validation

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