3
\$\begingroup\$

Currently I'm having a big form, with steps and a lot of complex fields. To make sure the user fills the required inputs and don't make them crazy pushing down the button submit when one required field on step 3 of 6 is no filled in, I decided to disable the button so they can see fast something is not right and check the form back.

My version is working but I'm not satisfied with the result because I'm sure it can be much better.

I simplify my form and I make a fiddle with my code to show the functionality:

You can check my code there.

HTML

<body>
  <form action="#myForm">
         Name * <input name="name1" placeholder="required" required> <br />
         Surname * <input name="surname" placeholder="required" required> <br />
         Age <input name="surname" placeholder="not-required"> <br />
         <button id="submitBtn" type="submit">Submit</button>
  </form>
</body>

Javascript + jQuery

   //This is the first load of is valid, just in case the user gets back again with old inputs
   //if all the required inputs are filled enable the button
   var isValid = true;
     $('input,textarea,select').filter('[required]:visible').each(function() {
      if ( $(this).val() === '' )
         isValid = false;
    });
    if( isValid ) {
       $('#submitBtn').prop('disabled', false);
    } else {
       $('#submitBtn').prop('disabled', true);
    };
    $('#submitBtn').click(function() {
        var isValid = true;
         $('input,textarea,select').filter('[required]:visible').each(function() {
          if ( $(this).val() === '' )
             isValid = false;
        });
        if( isValid ) {
           $("#myForm")[0].submit(); 
       };

    });

    //When a input is changed check all the inputs and if all are filled, enable the button
     $('input,textarea,select').change(function() {
        var isValid = true;
         $('input,textarea,select').filter('[required]:visible').each(function() {
          if ( $(this).val() === '' )
             isValid = false;
        });
        if( isValid ) {
           $('#submitBtn').prop('disabled', false);
        } else {
           $('#submitBtn').prop('disabled', true);
        };
    });

    //In case the user edits the button with firebug and removes disabled, check
    $('#submitBtn').click(function() {
         var isValid = true;
         $('input,textarea,select').filter('[required]:visible').each(function() {
          if ( $(this).val() === '' )
             isValid = false;
        });
        if( isValid ) {
           $("#myForm")[0].submit(); 
       };
    });
\$\endgroup\$
  • \$\begingroup\$ I've made some improvements in code, see updated DRY code here \$\endgroup\$ – Tushar Nov 25 '16 at 9:32
  • \$\begingroup\$ @Tushar thanks, make a answer to give you some points :) \$\endgroup\$ – Troyer Nov 25 '16 at 10:10
2
\$\begingroup\$

Sure you can DRYfy you code in several ways.

The most obvious point is that you're using 3 times the almost identical sequence of statements, so it should be factorized.

var isValid = true;
 $('input,textarea,select').filter('[required]:visible').each(function() {
  if ( $(this).val() === '' )
     isValid = false;
});
// then do something with "isValid"

Having isolated the sequence, we can already both simplify and improve its "core":

  • $(this).val() can be simpler and faster: this.value
  • but as pointed by @Tushar, it should be secured: this.value.trim();
    on the other hand, yes we might stop looping as soon as false (returning false rather than with break;, since each() is not a loop), but there can never be a significative lot of inputs so personally I'll give up, for the sake of readability
  • and finally the whole condition $(this).val() === '' can be yet simplified: !this.value.trim()

Now it's time to look at how to really factorize, i.e. make this sequence usable each time we need it to work.

But at the same time we may also think about how to make it of more general use. The idea here is to be able to work independently (while automatically) upon all or some of several forms.
It may be achieved by wrapping the sequence in a handler:

  • fired on any input element
  • looking for valid state of all input elements in its own form

So the whole sequence becomes a function like this:

function checkForm() {
  // here, "this" is an input element
  var isValidForm = true;
  $(this.form).find(':input[required]:visible').each(function() {
    if (!this.value.trim()) {
      isValidForm = false;
    }
  });
  // then do something with "isValidForm"... not stated yet
}

At this step we must decide how to make this handler be launched.
It worth mention that change is not a good choice, because it fires only when the element value had changed AND the element lost focus.
So for example, based on a full valid form, if user empties a required input the submit button remains enabled till navigated to another element!

It's why I suggest using keyup event instead (nevertheless if checkboxes or radios should also be involved, you'll have to also use change event).

For the sake of simple usage, I suggest affecting a distinguishing class to the submit button of each form you want to be involved, say monitored-btn.
So the way to make monitoring active will be:

$('.monitored-btn').closest('form').find(':input[required]:visible').keyup(checkForm);

This way, we know that our handler will be fired each time an element value changes.
So it's time to add what we want the handler to do depending on the form state, so it becomes:

function checkForm() {
  // here, "this" is an input element
  var isValidForm = true;
  $(this.form).find(':input[required]:visible').each(function() {
    if (!this.value.trim()) {
      isValidForm = false;
    }
  });
  $(this.form).find('.monitored-btn').prop('disabled', !isValidForm);
}

Now, each time an element value changes the involved submit button is enabled or not depending on the whole form state.
But when page was just loaded, the buttons are always all enabled regardless of anything else.
It's why we must immediately fire the handler as soon as it has been bound:

$('.monitored-btn').closest('form').find(':input[required]:visible')
  // bind the handler
  .keyup(checkForm)
  // immediately fire it to initialize buttons state
  .keyup();

Last point, regarding your care of user-hacking with Firebug (a bit strange, from my point of view, since if user can suppress the disabled attribute he can also do almost anything he wants, including directly submit through the console).

A simple way to also monitor that is to fire the same handler on form submit event, with two slight changes:

  • the handler expects this to be an input element, so we must call it like if it was a simple function (not a handler): checkForm.apply($(this).find(':input')[0]);
  • apart from enabling/desabling button, the handler should now return isValidForm, so we can use it to prevent defaut

It actually results in this final whole version:

function checkForm() {
  // here, "this" is an input element
  var isValidForm = true;
  $(this.form).find(':input[required]:visible').each(function() {
    if (!this.value.trim()) {
      isValidForm = false;
    }
  });
  $(this.form).find('.monitored-btn').prop('disabled', !isValidForm);
  return isValidForm;
}

$('.monitored-btn').closest('form')
  // indirectly bind the handler to form
  .submit(function() {
    return checkForm.apply($(this).find(':input')[0]);
  })
  // look for input elements
  .find(':input[required]:visible')
  // bind the handler to input elements
  .keyup(checkForm)
  // immediately fire it to initialize buttons state
  .keyup();

Here is a working example where I used two forms with distinct "required" exigences:

const inputSelector = ':input[required]:visible';
function checkForm() {
  // here, "this" is an input element
  var isValidForm = true;
  $(this.form).find(inputSelector).each(function() {
    if (!this.value.trim()) {
      isValidForm = false;
    }
  });
  $(this.form).find('.monitored-btn').prop('disabled', !isValidForm);
  return isValidForm;
}
$('.monitored-btn').closest('form')
  // in a user hacked to remove "disabled" attribute, also monitor the submit event
  .submit(function() {
    // launch checkForm for the first encountered input,
    // use its return value to prevent default if form is not valid
    return checkForm.apply($(this).find(':input')[0]);
  })
  .find(inputSelector).keyup(checkForm).keyup();
form {
  border: 1px solid #000;
  margin: 10px;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<body>
  <form action="#myForm">
         Name * <input name="myForm-name1" placeholder="required" required> <br />
         Surname * <input name="myForm-surname" placeholder="required" required> <br />
         Age <input name="myForm-surname" placeholder="not-required"> <br />
         <button id="myForm-submitBtn" class="monitored-btn" type="submit">Submit</button>
  </form>
  <form action="#otherForm">
         Name * <input name="otherForm-name1" placeholder="required" required> <br />
         Surname * <input name="otherForm-surname" placeholder="not-required"> <br />
         Age <input name="otherForm-surname" placeholder="required" required> <br />
         <button id="otherForm-submitBtn" class="monitored-btn" type="submit">Submit</button>
  </form></body>

\$\endgroup\$
  • \$\begingroup\$ We can break out of each. Use return false;, I used break which is wrong. \$\endgroup\$ – Tushar Nov 27 '16 at 12:24
  • \$\begingroup\$ @Tushar You're right! So I updated my answer accordingly (although in the current case I'm not partisan to take care of it: look at my edit). Also corrected my wrong citation: I'd confused Tushar and Troyer! :) \$\endgroup\$ – cFreed Nov 27 '16 at 15:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.