0
\$\begingroup\$

We have pages that contain 1-3 forms. Each form does have a submit button which is styled as inactive* as long as the user didn't change the form.

Now I need to:

  • remove the class from the button as soon as the form changes
  • remove the event listener from the form

I came up with this:

for (const form of Array.from(document.getElementsByClassName('form'))) {
    const formHandlerChange = event => {
        const submit = form.getElementsByClassName('disabled')[0];
        submit.classList.remove('disabled');
        form.removeEventListener('change', formHandlerChange);
    };

    form.addEventListener('change', formHandlerChange);
}

Is this a good solution? Is it a bad thing to create formHandlerChange for each form? Would it be better to get the form each time formHandlerChange is entered manually and have this function defined only once outside the loop?

* It's also disabled but I removed it for simplicity of this review.

\$\endgroup\$
  • \$\begingroup\$ Is it really necessary to remove the event handler after it triggers? In your case it would seem that you would just remove the disabled class more than once. What I prefer to do, instead of removing the event, is to check some condition. For example: if (submit.classlist.contains('disabled') { ... \$\endgroup\$ – Marc Rohloff Aug 29 '18 at 15:20
  • \$\begingroup\$ Well, I think that getting the DOM element over and over again only to check whether a class is set already seems like a lot of overhead? However, I could add an extra class to the form like enabled and test this for event.target, but this only makes the code more complex than needed, right? What do you think? @MarcRohloff \$\endgroup\$ – lampshade Aug 31 '18 at 9:31
  • \$\begingroup\$ I think that you're not going to notice a drop in performance for this kind of usage and you can always use Máté's suggestion of using currentTarget to avoid the lookup. I really don't think either direction would be wrong and you should do what you think is best. My rule of thumb is mostly just to be consistent across your code-base. \$\endgroup\$ – Marc Rohloff Aug 31 '18 at 19:04
  • \$\begingroup\$ I also just noticed your asterisk comment. If you are disabling the button then why use a class at all? You can target it in css with the :disabled and :enabled pseudo-classes. developer.mozilla.org/en-US/docs/Web/CSS/:disabled \$\endgroup\$ – Marc Rohloff Aug 31 '18 at 19:06
1
\$\begingroup\$

If you use event.currentTarget instead of form, you can declare the event handler function once and reuse it:

function formHandlerChange(event) {
    const form = event.currentTarget;
    const submit = form.querySelector('.disabled');
    submit.classList.remove('disabled');
    form.removeEventListener('change', formHandlerChange);
}

for (const form of document.querySelectorAll('form')) {
    form.addEventListener('change', formHandlerChange);
}

(Note: I also used querySelector() and querySelectorAll() as they are more flexible than the various getElementBy methods)

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. As asked in the OP, could you explain, why this is better than the "original"? Does this have a better performance? Is it easier to understand for other developers reading the code? \$\endgroup\$ – lampshade Aug 31 '18 at 9:25
  • \$\begingroup\$ Thanks for querySelector - didn't have this in mind. A note: For the code ro tun in Edge unfortunately you need Array.from(), see https://stackoverflow.com/questions/47287344/edge-15-throws-error-when-using-for-of-on-a-nodelist. \$\endgroup\$ – lampshade Aug 31 '18 at 9:28
  • 1
    \$\begingroup\$ I'd say this is a bit more readable, since it reduces the depth of nested code. Technically you're also saving a little bit of memory by only declaring the function once, but that's negligible. Generally, I just prefer to reuse as much code as I can. Also, I wasn't aware of the Edge bug, thanks for pointing that out. \$\endgroup\$ – Máté Safranka Aug 31 '18 at 9:31
  • \$\begingroup\$ Thanks again. Good point here with the level of nesting. Yeah, I only know that Edge quirk, because I asked that question on SO. :D \$\endgroup\$ – lampshade Aug 31 '18 at 9:34

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.