Why does my JavaScript forEach muliplication of two elements then setAttribute value to a third element code only run once?

Question

Background: I am creating a simple html order form for around 30 items using pure vanilla JavaScript (no jQuery) and basic CSS. It will not involve any online payments. New items may be added by my client and they will simply copy paste a block of code for an existing item in the html of the article and give it a new item name. They will not have access to the JavaScript. To avoid potential unwanted duplicates I am using only CSS classes and no element IDs. The form consists of a item name (text), a price and two input fields - one for quantity and one for total, with total being based dynamically on price*quantity as the form is filled.

My query: The quantity and form fields are pre-filled with zeros using a forEach loop and that works fine for all elements as expected. What I cannot get to work is a second loop which calculates the total price based on price x quantity for anything other than the first one it finds.

What am I missing? Is it something to do with the onblur="calculate" command in the quantity input element? I have to admit I found this part in someone else's code and haven't used it before. Perhaps I should be using something else.

I have looked at other questions in a similar vein on here (and elsewhere) but they're either using jQuery or other types of JS or tables and so on.

The code is below. TIA:

if (document.querySelector('.order-form')) {
    let qty = document.querySelectorAll('.qty');
  let total = document.querySelectorAll('.total');
    [].forEach.call(qty, function(zero) {
    zero.setAttribute('value','0');
    });
  [].forEach.call(total, function(zero) {
    zero.setAttribute('value','0');
    })
}
calculate = function() {
  let qty = document.querySelectorAll('.qty');
  [].forEach.call(qty, function() {
    let price = document.querySelector('.price').innerText;
    let newprice = parseFloat(price).toFixed(2);
    let qty = document.querySelector('.qty').value;
    let newtotal = parseFloat((newprice)*(qty)).toFixed(2);
    let total = document.querySelector('.total');
    total.setAttribute('value',newtotal);
    });
}

.order-form {
  margin: 16px;
  width: 100%;
}
.row {
  width: 100%;
}
.prod {
  margin-top: 16px;
}
.price {
  width: 10%; 
  display: inline-block;
}
.price:before {
  content: "Price: £";
}
.input-style {
  width: 70%; 
  display: inline-block;
}
label {
  padding: 0 16px;
}


    Subscription Receipt Book
  
    4.50
    
      Qty:
      Total: £
  
  
    General Receipt Book
    4.00
   
      Qty:
      Total: £
    
  
  
    Minutes book
    5.50
    
      Qty:
      Total: £

Rory McCrossan · Answer

The issue is because you're selecting all the .price, .qty and .total elements in the DOM in the calculate() function, not only the ones in the same row as the field which was updated.

To fix this you can simply use closest() to get the .row element relative to the .qty which the user updated, and select the necessary fields from there.

Also note that there's several other improvements which should be made to your code.

Don't use inline event handlers in your HTML, eg. onblur, onclick etc. They are outdated and not good practice. Bind your events unobtrusively in the JS using addEventListener() instead.
You can call forEach() directly on the collections returned by querySelectorAll(). This is now well supported so you don't need to mess around converting them to arrays first.
Set the value of the inputs at the time you generate them and append them to the DOM. If you do this after the elements have been created it's likely that they will appear empty for a split second which is unsightly, and should be avoided.
The .total fields default value should be set to 2dp, ie. 0.00, not just 0.
CSS should all be in an external stylesheet. Don't put it in your HTML in a style attribute.
Update the values of your inputs using the value property. You don't need to use setAttribute() for this.
The for attributes on your label elements won't do anything as their values don't match the id of any of the inputs. To workaround this, remove the for attributes and wrap the label elements around the input.
Make the .total fields readonly, otherwise users can edit them and set whatever price they like.

With all that said, try this:

document.querySelectorAll('.qty').forEach(el => {
  el.addEventListener('input', e => {
    const row = e.target.closest('.row');
    const qty = e.target.value;
    const price = row.querySelector('.price').textContent;
    row.querySelector('.total').value = parseFloat(price * qty).toFixed(2);
  });
});

.order-form {
  margin: 16px;
  width: 100%;
}

.row {
  width: 100%;
}

.prod {
  margin-top: 16px;
}

.price {
  width: 10%;
  display: inline-block;
}

.price:before {
  content: "Price: £";
}

.input-style {
  width: 70%;
  display: inline-block;
}

label {
  padding: 0 16px;
}


  Subscription Receipt Book
  
    4.50
    
      
        Qty:
        
      
      
        Total: £
        
      
    
  
  
    General Receipt Book
    4.00
    
      
        Qty:
        
      
      
        Total: £
        
      
    
  
  
    Minutes book
    5.50
    
      
        Qty:
        
      
      
        Total: £

Why does my JavaScript forEach muliplication of two elements then setAttribute value to a third element code only run once?

1 Answers