Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Pricing Component using SCSS and Vanilla Javascript

Nicoleโ€ข 360

@nicole-tuznik

Desktop design screenshot for the Pricing component with toggle coding challenge

This is a solution for...

  • HTML
  • CSS
  • JS
2junior
View challenge

Design comparison


SolutionDesign

Solution retrospective


No specific questions, but feedback is always appreciated! :)

Community feedback

tomthestromโ€ข 165

@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

0
Samuel Palaciosโ€ข 615

@samuelpalaciosdev

Posted

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

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord