Reputation: 123
I've created a working price toggler, that also has an upsell remover/adder. While it works well, I'd like to understand how I could do it better, because I'm having to essentially run two compound if/else statements to account for the duration toggle and the upsell toggle.
HTML:
<div>
<p>Annually</p>
<!-- Toggle Button -->
<label for="toggle">
<!-- toggle -->
<div class="relative">
<!-- hidden input -->
<input
id="toggle"
type="checkbox"
class="hidden"
onclick="setPrice()"
data-sc-monthly="59"
data-both-monthly="79"
data-bc-monthly="20"
/>
<!-- line -->
<div></div>
<!-- dot -->
<div class="toggle_dot"></div>
</div>
</label>
<p>Monthly</p>
</div>
<div>
<div>
<h3>Main Report</h3>
<h4>$<span id="price">790</span></h4>
<p><span class="annual">ANNUALLY</span><span class="month hidden">MONTHLY</span></p>
<div>
<input
id="package"
type="checkbox"
onclick="setPackage()"
checked
/>
<h5>Include our special secondary coverage for just $<span id="bcValue">200</span></h5>
</div>
<a class="btn btn-success">
JOIN NOW
</a>
</div>
</div>
JAVASCRIPT:
<script>
const pricing = document.querySelector('#toggle');
var single = pricing.dataset.scMonthly;
var both = pricing.dataset.bothMonthly;
var bc = pricing.dataset.bcMonthly;
var price = both;
function setPackage(){
if (document.getElementById("package").checked == true) {
if (document.getElementById("toggle").checked == true) {
var price = both;
document.getElementById("price").innerHTML = price;
} else {
var price = both * 10;
document.getElementById("price").innerHTML = price;
}
} else {
if (document.getElementById("toggle").checked == true) {
var price = single;
document.getElementById("price").innerHTML = price;
} else {
var price = single * 10;
document.getElementById("price").innerHTML = price;
}
}
}
function setPrice() {
var x = document.querySelectorAll('.annual');
var y = document.querySelectorAll('.month');
for (var i = 0; i < x.length; i++) {
if (document.getElementById("toggle").checked == true) {
x[i].classList.add('hidden');
y[i].classList.remove('hidden');
if (document.getElementById("package").checked == true) {
document.getElementById("price").innerHTML = both;
} else {
document.getElementById("price").innerHTML = single;
}
document.getElementById("bcValue").innerHTML = bc;
} else {
x[i].classList.remove('hidden');
y[i].classList.add('hidden');
if (document.getElementById("package").checked == true) {
document.getElementById("price").innerHTML = both * 10;
} else {
document.getElementById("price").innerHTML = single * 10;
}
document.getElementById("bcValue").innerHTML = bc * 10;
}
}
}
</script>
As you can see, each function has to check if the parameters controlled by the other function are set or not, and make adjustments accordingly.
My JS-fu is not strong enough yet to figure out how to improve this code overall.
Upvotes: 1
Views: 70
Reputation: 123
Here's what I wound up using based on an answer from CodeReview:
const pricing = document.querySelector('#toggle');
var single = pricing.dataset.scMonthly;
var both = pricing.dataset.bothMonthly;
var bc = pricing.dataset.bcMonthly;
var price = both;
for (const id of ["toggle", "package"]) {
document.getElementById(id).addEventListener("click", update);
}
function hideAll(query) {
for (const element of document.querySelectorAll(query)) {
element.classList.add("hidden");
}
}
function showAll(query) {
for (const element of document.querySelectorAll(query)) {
element.classList.remove("hidden");
}
}
function update() {
const package = document.getElementById("package").checked ? both : single;
let factor;
if (document.getElementById("toggle").checked) {
factor = 1;
hideAll(".annual");
showAll(".month");
} else {
factor = 10;
showAll(".annual");
hideAll(".month");
}
document.getElementById("price").innerHTML = package * factor;
document.getElementById("bcValue").innerHTML = bc * factor;
}
Upvotes: 1
Reputation: 1155
Since you are always updating the same elements, you can work out the price before assigning the result:
function setPackage(){
var price = document.getElementById("package").checked ? both : single;
if (document.getElementById("toggle").checked) {
price *= 10;
}
document.getElementById("price").innerHTML = price;
}
For the second function, you should move the logic that updates the price out of the loop to avoid repeating the same action unnecessarily.
function setPrice() {
var showMonthly = document.getElementById("toggle").checked;
var hideClass = showMonthly ? '.annual' : '.monthly'; // choose which info to hide
// display the currently hidden info
document.querySelectorAll('.hidden').forEach(function (el) {
el.classList.remove('hidden');
});
document.querySelectorAll(hideClass).forEach(function (el) {
el.classList.add('hidden');
});
var price = document.getElementById("package").checked ? both : single;
var multiplier = showMonthly ? 1 : 10;
document.getElementById("price").innerHTML = price * multiplier;
document.getElementById("bcValue").innerHTML = bc * multiplier;
}
Upvotes: 2