if (i1 >= kg + " g")
If you append " g" to kg, you’re comparing i1 >= "1000 g", which in the case of x === 2 is 1400 >= "1000 g". Since one of the operands is a string, it will be coerced to NaN, and every comparison, no matter what i1 is, will return false. You want:
if (i1 >= kg)
And then, append + " kg" to the resulting string instead of + " g".
While the above solves and explains the issue, I will add a different, simpler approach, along with some advice:
- Don’t use inline
onclick attributes. Instead use standard addEventListener.
- There’s no point in using the unary
+ before i1 etc. when concatenating " g". They’re already numbers, and will become part of a string, anyway.
- Don’t use this many IDs. It’s far simpler to wrap all those
<div>s in a single <div> with one ID instead:
<div id="ing">
<div>700 g</div>
<div>200 g</div>
<div>0.4 g</div>
<div>20 ml</div>
<div>1 </div>
</div>
// Get initial amounts and units from <div>s here.
const amounts = Array.from(document.getElementById("ing").children, (child) => {
let [amount, unit] = child.innerText.split(" ");
return {
amount: Number(amount),
unit: unit || "", // If there is no space in the string (e.g. just "10"), then `unit` should be the empty string (without `|| ""` it will be `undefined`).
div: child
};
}),
button = document.getElementById("button"), // Add an ID "button" to the “Calculate” button, instead of an `onclick` attribute
kg = 1000;
function multiplyBy(){
const serves = Number(document.getElementById("serves").value); // You use this as a number, but .value returns a string. Better convert it immediately, to avoid future errors.
amounts.forEach(({amount, unit, div}) => {
const multipliedAmount = amount * serves,
useKg = unit === "g" && multipliedAmount >= kg; // This condition holds iff the multiplied amount is at least 1000 g.
div.innerText = `${Number((multipliedAmount / (useKg ? kg : 1)).toFixed(1))} ${useKg ? "kg" : unit}`;
});
}
button.addEventListener("click", multiplyBy); // Assign the click handler here.
// Alternatively: ditch the button and use a change listener on the `serves` input instead:
// document.getElementById("serves").addEventListener("change", multiplyBy);
This will also take care of different units remaining their own units, instead of becoming g. See the following snippet for a working example:
// Get initial amounts and units from <div>s here.
const amounts = Array.from(document.getElementById("ing").children, (child) => {
let [amount, unit] = child.innerText.split(" ");
return {
amount: Number(amount),
unit: unit || "", // If there is no space in the string (e.g. just "10"), then `unit` should be the empty string (without `|| ""` it will be `undefined`).
div: child
};
}),
button = document.getElementById("button"), // Add an ID "button" to the “Calculate” button, instead of an `onclick` attribute
kg = 1000;
function multiplyBy(){
const serves = Number(document.getElementById("serves").value); // You use this as a number, but .value returns a string. Better convert it immediately, to avoid future errors.
amounts.forEach(({amount, unit, div}) => {
const multipliedAmount = amount * serves,
useKg = unit === "g" && multipliedAmount >= kg; // This condition holds iff the multiplied amount is at least 1000 g.
div.innerText = `${Number((multipliedAmount / (useKg ? kg : 1)).toFixed(1))} ${useKg ? "kg" : unit}`;
});
}
button.addEventListener("click", multiplyBy); // Assign the click handler here.
// Alternatively: ditch the button and use a change listener on the `serves` input instead:
// document.getElementById("serves").addEventListener("change", multiplyBy);
<input type="number" value="1" min="1" max="100" id="serves"/>
<input type="button" id="button" Value="Calculate"/>
<br/><br/>
<div id="ing">
<div>700 g</div>
<div>200 g</div>
<div>0.4 g</div>
<div>20 ml</div>
<div>1 </div>
</div>
