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

Fix Input Fields validation message #438

Draft
wants to merge 4 commits into
base: v2-dev
Choose a base branch
from

Conversation

Jerit3787
Copy link

@Jerit3787 Jerit3787 commented Nov 29, 2023

Proposed changes

Fixes #437

This pull request aims to fix input fields validation. This feature were removed during the transition to vanila JS. This code has been refactored from older code which composed from jQuery.

Screenshots (if appropriate) or codepen:

Before (v2.0.3-alpha) - Preview:
image

After (branch textfield-fix) - Preview:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My commit messages follows the conventional commit format
  • My change requires a change to the documentation, and updated it accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Jerit3787 Jerit3787 changed the base branch from main to v2-dev November 29, 2023 09:48
}
} else {

if (textarea.classList.contains('validate')) {

Choose a reason for hiding this comment

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

Wouldn't it be sufficient to check weather or not the text are has a "data-length" Attribute?
If it has, it indicates that the user wants the textarea to be validated.

Copy link
Author

@Jerit3787 Jerit3787 Dec 1, 2023

Choose a reason for hiding this comment

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

Yeah, that could work. I couldn't find anything referencing the validate attribute other than this part. As I stated, this is the code from previous version of it. Not sure if it could be impacting anything else so far.

Referencing here from v1.0.0:

M.validate_field = function(object) {
    let hasLength = object.attr('data-length') !== null;
    let lenAttr = parseInt(object.attr('data-length'));
    let len = object[0].value.length;

    if (len === 0 && object[0].validity.badInput === false && !object.is(':required')) {
      if (object.hasClass('validate')) {
        object.removeClass('valid');
        object.removeClass('invalid');
      }
    } else {
      if (object.hasClass('validate')) {
        // Check for character counter attributes
        if (
          (object.is(':valid') && hasLength && len <= lenAttr) ||
          (object.is(':valid') && !hasLength)
        ) {
          object.removeClass('invalid');
          object.addClass('valid');
        } else {
          object.removeClass('valid');
          object.addClass('invalid');
        }
      }
    }
  };

From the code above, this seems linked to the textbox that have character count. I might have to recheck the CSS behavior.

@@ -81,6 +116,20 @@ export class Forms {
static Init(){
document.addEventListener("DOMContentLoaded", () => {

document.addEventListener('change', (e: KeyboardEvent) => {

Choose a reason for hiding this comment

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

Can't we attach the validate Function to the input field directely?

Copy link
Author

@Jerit3787 Jerit3787 Dec 1, 2023

Choose a reason for hiding this comment

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

yeah kinda, here I find kinda wierd checking for input element inside the function and outside of it. But, to be frankly speaking, the old jQuery code only checks for the length and applies the validate_field immediately.

Referencing here from v1.0.0:

$(document).on('change', input_selector, function() {
      if (this.value.length !== 0 || $(this).attr('placeholder') !== null) {
        $(this)
          .siblings('label')
          .addClass('active');
      }
      M.validate_field($(this));
    });

Since the textbox are now being handled by CSS, I think we can drop the length check. Again, I need to confirm the CSS behavior again.

@Jerit3787 Jerit3787 marked this pull request as draft December 4, 2023 14:51
@wuda-io
Copy link
Member

wuda-io commented Dec 5, 2023

Hello @Jerit3787
Thank you for your PR, here the intention that the validation function was removed:
The validation should be done on the server-side if possible. It was removed because it is not a good practice and to prevent bad security. Also to force the users to write their own custom validation function.

So as a solution maybe we could add a custom function as @Lord-Leonard stated. And the property could then be called untrustedValidationFunction.

@Jerit3787
Copy link
Author

Jerit3787 commented Dec 5, 2023

Hello @Jerit3787 Thank you for your PR, here the intention that the validation function was removed: The validation should be done on the server-side if possible. It was removed because it is not a good practice and to prevent bad security. Also to force the users to write their own custom validation function.

So as a solution maybe we could add a custom function as @Lord-Leonard stated. And the property could then be called untrustedValidationFunction.

If that's the case, the docs need to be updated accordingly as well. The docs contains features that are removed which I thought the feature was accidentally remove without notice. I noticed there were forms of textfield that contains error, not sure what are they for. We could add this as a notice that these needed to be handled manually.

p/s: the reason I think this is needed is because I don't use this library on server-sided processing such as node.js. Having these features manually coded ultimately defeats the whole security purpose. And this just disables the regular html textbox validation if moved to server-sided. Just my two cents.

@Jerit3787 Jerit3787 closed this Dec 5, 2023
@wuda-io wuda-io reopened this Dec 6, 2023
@wuda-io
Copy link
Member

wuda-io commented Dec 6, 2023

Hey @Jerit3787
I dont wanted to be mean, your PR is still cool and good! I found a good article how we should handle this situation and how we can solve this. Check out the following article: https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation

Maybe we can get it to work without JavaScript in first place. Only via CSS pseudo class ``:invalid```
And on top of that we could add a custom callback function which checks for special properties or so.
What do you think?

@Jerit3787
Copy link
Author

Hey @Jerit3787 I dont wanted to be mean, your PR is still cool and good! I found a good article how we should handle this situation and how we can solve this. Check out the following article: https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation

Maybe we can get it to work without JavaScript in first place. Only via CSS pseudo class ``:invalid``` And on top of that we could add a custom callback function which checks for special properties or so. What do you think?

Yeah, sorry if my message looks like I’m mad :). Maybe we could look at it but currently I don’t really have the time to finish this yet. I’ll try work on it later.

@Jerit3787
Copy link
Author

I've tried some examples using css instead of js. But, there is some concerns.

  1. Textfield will show green if the field is empty.
    image
    Thus, the data-correct will be need to be removed to avoid confusion.

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.

[Bug]: input field validation doesnt work
3 participants