4
\$\begingroup\$

The following code allows users to enter multiple optional quantity fields after first filling in a value of the current quantity input. If the input field contains a value it will then display the next quantity field, if not, it will remove/not display it.

The code is currently working as intended but I don't believe that it follows the DRY principle very well. Is there a way to make this more succinct?

function quantityCheck() {
    var q1 = document.getElementById("Quantity1Value").value;
    if (q1 !== "") {
        document.getElementById("Quantity2").style.display = "";
    } else {
        document.getElementById("Quantity2").style.display = "none";
    }

    var q2 = document.getElementById("Quantity2Value").value;
    if (q2 !== "") {
        document.getElementById("Quantity3").style.display = "";
    } else {
        document.getElementById("Quantity3").style.display = "none";
    }

    var q3 = document.getElementById("Quantity3Value").value;
    if (q3 !== "") {
        document.getElementById("Quantity4").style.display = "";
    } else {
        document.getElementById("Quantity4").style.display = "none";
    }
}
<div>
    <span>Quantity 1</span>
    <input type="number" id="Quantity1Value" oninput="quantityCheck()" required>
</div>

<div id="Quantity2" style="display:none">
    <span>Quantity 2</span>
    <input type="number" id="Quantity2Value" oninput="quantityCheck()">
</div>

<div id="Quantity3" style="display:none">
    <span>Quantity 3</span>
    <input type="number" id="Quantity3Value" oninput="quantityCheck()">
</div>

<div id="Quantity4" style="display:none">
    <span>Quantity 4</span>
    <input type="number" id="Quantity4Value">
</div>

\$\endgroup\$
  • 3
    \$\begingroup\$ Is it working as intended? What if Quantity 1, 2 and 3 are shown and we remove the value from 1, what should happen? Currently, 2 gets hidden but 3 remains. Don't you require a cascading effect? \$\endgroup\$ – dfhwze Aug 13 at 17:58
  • 2
    \$\begingroup\$ @dfhwze that's a good question. I assumed perhaps more than I should have, as my approach only works for two individual elements (as the original code does). If it needs to cascade, my approach wouldn't work. \$\endgroup\$ – Bodacious Aug 13 at 18:13
  • 1
    \$\begingroup\$ @Bodacious let's wait and see what OP has to say.. Your answer is already a valid one for the current status of the question. And since you have answered already, I am not sure OP should answer my question in the comment anymore. \$\endgroup\$ – dfhwze Aug 13 at 18:16
  • 1
    \$\begingroup\$ @dfhwze Thanks for pointing that out, I would prefer it to cascade. Should I update my question with this markup or would it be better to ask a new question? codepen.io/Painguin/pen/VoGzLb \$\endgroup\$ – Painguin Aug 13 at 18:34
  • 1
    \$\begingroup\$ @Bodacious I would ask a follow-up. Your current question has been answered. And updating the current question would invalidate the answer. \$\endgroup\$ – dfhwze Aug 13 at 18:35
4
\$\begingroup\$

It would be better to do your evaluation only for the particular elements that could be affected each time rather than to reevaluate all of them on any change in any input.

We can rewrite this to pass along the element that triggered the event and the ID of the element that we want to update the style on. Take a look at this:

function quantityCheck(eventElement, quantity) {
	//try to use const or let instead of var. 
	//by passing 'this', we now have the target element that triggered the event,
	//so we only need to grab the one we want to change.
	const quantToChange = document.getElementById(quantity);

	//we can also use a tertiary function to make our change to shorten things a bit.
	quantToChange.style.display = eventElement.value.length ? '' : 'none';
}
 <div>
    <span>Quantity 1</span>
    <input type="number" id="Quantity1Value" oninput="quantityCheck(this, 'Quantity2')" required>
</div>

<div id="Quantity2" style="display:none">
    <span>Quantity 2</span>
    <input type="number" id="Quantity2Value" oninput="quantityCheck(this, 'Quantity3')">
</div>

<div id="Quantity3" style="display:none">
    <span>Quantity 3</span>
    <input type="number" id="Quantity3Value" oninput="quantityCheck(this, 'Quantity4')">
</div>

<div id="Quantity4" style="display:none">
    <span>Quantity 4</span>
    <input type="number" id="Quantity4Value">
</div>

\$\endgroup\$
4
\$\begingroup\$

Put styles in stylesheet, and use semantic markup. It's easier to think about the problem if it is a list of input controls.

You would most probably want to reset the hidden inputs, because in case a previous input value is removed, and then the form is submitted, those hidden input values would still get included.

Adding a listener on a form propagates it to contained controls so that the event target is the control being manipulated. So in the example below, in case of other controls, the listener would need to check if the target is something that it needs to act upon or not.

In the example below, I have changed the behavior so that on input, the next input label's hidden CSS class is removed, and on removing content from input, all succeeding input labels get that class added, and also resetting their value.

document.addEventListener('DOMContentLoaded', () => {
  const form = document.forms[0] // NOTE: change if multiple forms
  const inputs = Array.from(form.elements)
  // ^NOTE: use a selector or filter if other controls

  const hideAndReset = input => {
    input.parentElement.classList.add('hidden')
    input.value = ''
  }

  const showNext = ({target}) => {
    const nextIndex = inputs.indexOf(target) + 1

    if(nextIndex == inputs.length)
      return false

    if(target.value)
      inputs[nextIndex].parentElement.classList.remove('hidden')
    else
      inputs.slice(nextIndex).forEach(hideAndReset)
  }

  form.addEventListener('input', showNext)
})
label { display: block }
.hidden { display: none }
<form>
  <label>
    <span>Quantity 1</span>
    <input id="qty1" type="number" required>
  </label>

  <label class="hidden">
    <span>Quantity 2</span>
    <input id="qty2" type="number">
  </label>

  <label class="hidden">
    <span>Quantity 3</span>
    <input id="qty3" type="number">
  </label>

  <label class="hidden">
    <span>Quantity 4</span>
    <input id="qty4" type="number">
  </label>
</form>

\$\endgroup\$

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.