room-homepage

Community feedback

• 

  P

  Habeeb Kareem•640

  @olaide-hokPosted 4 days ago

  Great work here!

  Your JS contained

  ✅ Readable Code

  • The code is well-structured with meaningful variable names (carousel_pics, carousel_txts, etc.).
  • The use of function encapsulation (e.g., updateCarousel(), carouselNext(), showMenu()) improves clarity.

  ✅ Accessibility Considerations

  • The use of tabIndex, aria-hidden, and keydown event listeners for keyboard navigation enhances usability for screen readers.

  ✅ Separation of Concerns

  • The carousel logic and navigation logic are separated into different sections, making the code easier to manage.

  ---

  Some areas for Improvement includes

  🔹 1. Issue with captions.forEach(e => { e.shop_now = e.querySelector('.shop-now'); });

  Problem:

  • shop_now is being assigned as a new property to e (a DOM element), which is not a standard practice and could lead to unexpected behavior.

  Suggested Fix:
  Use a Map or a dataset attribute instead of modifying the DOM object:

  const shopNowButtons = new Map();
  captions.forEach(e => {
      shopNowButtons.set(e, e.querySelector('.shop-now'));
  });
  

  Then, access it like this:

  shopNowButtons.get(txt).tabIndex = 0;
  

  🔹 2. Inefficient updateCarousel() Function

  Problem:

  • The function recalculates img_width and caption_width every time it runs, which is unnecessary.

  • Instead of looping through all images and captions every time, we can optimize how we update styles.

  Suggested Fix: Cache widths once and reuse them like below:

  const imgWidth = pictures[0].clientWidth;
  const captionWidth = captions[0].clientWidth;
  
  const updateCarousel = (count) => {
      carousel_pics.style.transform = `translateX(-${count * imgWidth}px)`;
      carousel_txts.style.transform = `translateX(-${count * captionWidth}px)`;
  
      pictures.forEach((pic, i) => {
          const txt = captions[i];
          const isActive = i === count;
          pic.setAttribute('aria-hidden', !isActive);
          txt.setAttribute('aria-hidden', !isActive);
          shopNowButtons.get(txt).tabIndex = isActive ? 0 : -1;
      });
  };
  

  • This reduces unnecessary calculations and improves performance.

  🔹 3. Event Listeners Could Be More Efficient

  Problem:

  • Multiple click and keydown event listeners for each button could be refactored into a single handler.

  • Using e.key === "Enter" is preferred over e.code == 'Enter' for better compatibility.

  Suggested Fix: Use a single function for event handling:

  const handleKeyEvent = (event, action) => {
      if (event.type === "click" || event.key === "Enter") {
          action();
      }
  };
  
  left_arrow.addEventListener("click", carouselPrev);
  left_arrow.addEventListener("keydown", (e) => handleKeyEvent(e, carouselPrev));
  right_arrow.addEventListener("click", carouselNext);
  right_arrow.addEventListener("keydown", (e) => handleKeyEvent(e, carouselNext));
  
  

  On the CSS side,

  1. In desktop viewport you can apply a width: 1440px to the main tag. This would keep the content in place for viewport greater than 1440px.
  2. You can still refactor the section tag with about class to a grid-template-column: repeat(3, 1fr); for desktop viewport and grid-template-column: 1fr; for mobile. The padding could be adjusted for mobile and desktop also.

  Overall, well done. Happy coding!

  Marked as helpful

  0

  ---

  

  P

  toshirokubota•720

  @toshirokubotaPosted 4 days ago

  @olaide-hok Thank you for your detailed feedbacks. I will keep your advices in mind for my continuing development. I really appreciate your help! Happy coding to you, too!

  1