JavaScript toggle form elements

Question

Fellow JS devs, I have come across a fairly simple question which has puzzled me. When the user submits a form I disable all inputs and submit button changes to a loading animation (fa-spinner from fontawesome). Once the form submit is complete then I restore the previous state, and enable everything back.

My question comes to making this code more concise and deliberate in a simple toggle function:

function toggleFormElements(toggle, elArr) {
 if (toggle == 'disable') {
 elArr.btn.innerHTML = '<i class="fas fa-spinner fa-spin"></i>';
 elArr.btn.setAttribute('disabled', 'disabled');
 // inputs
 elArr.fields.forEach((field, i) => {
 field.setAttribute('disabled', 'disabled');
 });
 } else if (toggle == 'enable') {
 elArr.btn.innerHTML = 'Submit';
 elArr.btn.removeAttribute('disabled');
 // inputs
 elArr.fields.forEach((field, i) => {
 field.removeAttribute('disabled');
 });
 }
}

Would it be possible to make this code more readable or simply shorter?

Answer 1

arr.forEach

I would consider using a simple for loop to iterate over the elArr (perhaps consider a bit better name, like elementsArray or just elements). This StackOverflow post goes over some of the benefits of doing this.

You can also use the disabled attribute directly.

for (let i = 0; i < elArr.length; i++) {
 elArr[i].disabled = true; // or false to remove it
});

I would also personally prefer to have a boolean argument like isToggled instead of expecting a string, so you can do something like below. In my opinion a boolean is less likely to result in an error (e.g. a misspelled string).

if (isToggled) {
 ...
} else {
 ...
}

The comment // inputs doesn't really achieve anything, I would suggest to remove it completely.

• sam: Haha yes the // inputs comment was just for this question. I don't actually have it on there 😁
• sam: That's for the answer, it actually makes a lot of sense, especially with the bool vs a string

Answer 2

That's not a toggle

• Toggle has a very specific meaning and comes with the benefit of not having to supply the new state when called. Your function is not a toggle and should be called something along the lines of setFormDisableState(formElements, disable = true) or setFormState(formElements, disable)

Argument order

• If not using a default parameter use the right to left assignment rule when defining argument order. Eg assignment is right to left for a = b. Your function is assigning a state to elements in the object elArr, the elArr should be to the left of the toggle.

Danger! implied type

• elArr is not an array. That is most defiantly a bad name as it implies the incorrect type.

The shortest form.

• For all attributes defined in the DOM set the values directly. You only use the attribute functions for attributes not defined by the DOM. Using the longer version to do anything in code adds unneeded noise.

Keep the source DRY

• Don't Repeat Yourself (code). The field iterators elArr.fields.forEach are almost identical, with only the state changing.

• You have two if statements. You don't need to do the second test. If not disabling then must be enabling.

Alternatives

function toggleFormState(formDesc) { // Desc is short for description
 const newState = ! formDesc.btn.disabled; 
 formDesc.btn.innerHTML = newState ? '<i class="fas fa-spinner fa-spin"></i>' : "Submit";
 formDesc.btn.disabled = newState;
 for (const element of formDesc.fields) { element.disabled = newState }
}

Or

function setFormState(formDesc, disable) { 
 formDesc.btn.innerHTML = disable ? '<i class="fas fa-spinner fa-spin"></i>' : "Submit";
 formDesc.btn.disabled = disable;
 for (const element of formDesc.fields) { element.disabled = disable }
}

• Sᴀᴍ Onᴇᴌᴀ: nice use of for...of in the alternatives