FAQ Accordion Card

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop is lot smaller than the design as well for the mobile state. Adjusting those sizes will be really great.

  Also congrats for your first js solution I guess? :>>

  Some suggestions would be:

  • Add an additional aria-hidden="true" on the box img so that it will be hidden totally for all users. Same goes for the dropdown img.
  • Adding a cursor: pointer to the desktop layout for the button would be really great.
  • On the button it would be great to add an aria-expanded="false" as a default attribute. When the user toggles the button you would set the value to true and vice versa. This way, you users will be informed that the button has expanded/shown something. You will be needing to use .setAttribute method.

  Also for the js, a tip, adding a global onclick event on the window is not great, for me, since by doing this, every other clicks activates it even though you only want to click the button right.

  Try some adding console.log inside of the window.onclick function, even if not clicking the button you are firing it.

  So it would be better if:

  1. Only query the elements that you need, hence the button
  2. Only apply events on the queried elements.

  Your javascript would look like, in my refactored version:

  const accordionButtons = document.querySelectorAll(".dropbtn");
  
  const addActiveClass = event => event.target.classList.toggle("active");
  
  accordionButtons.forEach(button => button.addEventListener("click", addActiveClass));
  

  You only need this 3 line. Then you might think, "okay I am adding an active class to the button, how would I transition the content".

  Well since your button and the content are sibling on the markup, you just need this single css property.

  .dropbtn.active + .dropdown-content {
      display: block;
  }
  
  .dropdown.active .arrow {
    transform: rotate(180deg);
  }
  

  This way, the code is minimized and you utilized the positioning of your html element wonderfully. This is really great especially when you design the markup to be easy to handle, that is proper markup is important and you nailed it on this one.

  Though you might need to adjust on the code since it might be new to you. .forEach is an array method that iterates for every element in an array. .querySelectorAll queries "all" element , creating an array that gets all the element.

  and on this line:

  accordionButtons.forEach(button => button.addEventListener("click", addActiveClass));
  

  You might see that after the click I added the function of addActiveClass. Doing this way, the event that the element has , will be passed on to the function you supplied, but don't forget that you are only using the function as callback and do not invoke it by using () on the end.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Grand Marcell•130

  @GMarcellPosted about 3 years ago

  @pikamart Thank you for the suggestion i'll gonna fix this soon

  1