Interactive Rating Component

Community feedback

• 

  P

  Huy Phan•3,420

  @huyphan2210Posted 14 days ago

  Hi @rukhulkirom,

  I've reviewed your solution, and I'd like to share some suggestions for improvement:

  1️⃣ Use a CSS Naming Convention (e.g., BEM)

  Using a convention like BEM can make your CSS more structured and easier to maintain. For example, instead of having .card and .thankyou-card, you could use:

  .card { /* Base styling */ }
  .card--thank-you { /* Modifier for thank-you card */ }
  

  This keeps your styles consistent and scalable.

  2️⃣ Remove Unnecessary id Attributes

  You assign ids to your cards but style them using classes instead. Since the ids aren’t necessary, I recommend removing them to avoid redundancy.

  3️⃣ Use Semantic HTML for Better Accessibility

  Instead of relying heavily on <div>, consider using semantic elements:

  • Use <article> instead of <div> for your cards.
  • Wrap <img> elements inside <figure> (or <picture> for responsive images).
  • You’ve correctly used <ul> with <li> for the .rating-btns—great! However, add type="button" to each .rating-btn for better behavior.

  4️⃣ Wrap Your Rating System Inside a <form>

  Instead of handling each button click separately, wrap your <ul class="rating-buttons"> and .submit-btn inside a <form>, then:

  • Set type="submit" on .submit-btn.
  • Handle the form’s submit event instead of attaching a click event to each rating button.

  5️⃣ Optimize Your JavaScript with Event Delegation

  If you're keeping the click event on each rating button instead of using a <form>, you can still make your JavaScript more efficient using Event Delegation:

  Before (Less Efficient)

  ratingBtn.forEach((button) => {
    button.addEventListener("click", () => {
      ratingBtn.forEach((btn) => btn.classList.remove("active"));
      button.classList.add("active");
      selectedRating = button.getAttribute("data-value");
    });
  });
  

  After (More Efficient with Event Delegation)

  document.querySelector(".rating-buttons").addEventListener("click", (event) => {
    const button = event.target.closest(".rating-btn");
    if (!button) return; // Ignore clicks outside buttons
  
    document.querySelectorAll(".rating-btn").forEach((btn) => btn.classList.remove("active"));
    button.classList.add("active");
    selectedRating = button.getAttribute("data-value");
  });
  

  ✅ Why is this better?

  • Only one event listener instead of one per button.
  • Less memory usage and improved performance.

  Hope this helps!

  0