Pricing Component using SCSS and Vanilla Javascript

Community feedback

• 

  tomthestrom•165

  @tomthestromPosted over 3 years ago

  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

  0
• 

  Samuel Palacios•615

  @samuelpalaciosdevPosted over 3 years ago

  Hi, Nicole👋

  Well done on this challenge! Your solution looks good and it scales pretty well👍

  I only suggest some things 😉:

  • Adding a transition to the toggle. I'd add a transition: .5s; on the #toggle::after element, it make it feel more smooth.

  • You don't have a h1 on this project as it stands. Having your headings on order is not really a rule, but the h1 it's one of the most important tags on an webpage . For this project, it would be where you've got the h5 heading.

  I hope this would help you, have a nice day, keep coding!💙

  0