@tomthestrom
Posted
Hey Nicole,
I was looking at your JS code. It's nice and readable, just a few things that are mostly a matter of taste.
This whole code could be wrapped in an IIFE
to avoid polluting the global namespace. It's a basic app and does no harm doing it the way you do, but it's a good practice to think if you really need to have globals before using them (build good habits from the beginning).
let monthlyPrice
and let annualPrice
should be const
, they are not reassigned anywhere.
const toggle = document.querySelector("#toggle");
could be a getElementById
, it tells better what you're doing since querySelector
is more of a general tool.
querySelectorAll
based on class names are not the best, it's a better practice to use data
attributes, again at this scale of an app it doesn't matter that much, just something to be aware of.
toggle.addEventListener("click", function () {
if (toggle.checked) {
for (i = 0; i < annualPrice.length; i++) {
annualPrice[i].style.display = "none";
}
for (i = 0; i < monthlyPrice.length; i++) {
monthlyPrice[i].style.display = "block";
}
} else {
for (i = 0; i < annualPrice.length; i++) {
annualPrice[i].style.display = "block";
}
for (i = 0; i < monthlyPrice.length; i++) {
monthlyPrice[i].style.display = "none";
}
}
});
This could be shortened for readability and to reduce repetitiveness to:
toggle.addEventListener("click", function () {
for (i = 0; i < annualPrice.length; i++) {
annualPrice[i].style.display = this.checked ? "none" : "block";
}
for (i = 0; i < monthlyPrice.length; i++) {
monthlyPrice[i].style.display = this.checked ? "block" : "none";
}
});
You could also use Array.map()
or Array.forEach()
instead of those for
loops to make it more declarative.
Your github repo shouldn't include the .DS_Store
file and the package-lock.json
.
Have a good day,
Tom