Mobile-first solution using BEM, Flexbox, SCSS, and Vanilla JS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Desktop layout looks great, it is responsive and the mobile layout looks great as well.

  I haven't tackled this challenge yet so I can't quite answer your question :>. But for suggestions on the site, here are some:

  • Avoid using height: 100vh on a large container like the body tag as this limits the element's height based on the remaining screen's height. Instead use min-height: 100vh, this takes full height but lets the element expand if needed.
  • On the main you don't really need to set the margin on the top and bottom part, just add padding the body and let the content be centered by the body's flebox.
  • Decorative image must be hidden at all times by using alt="" and extra aria-hidden="true" attribute on the img tag, images like the box and the vector-women.
  • Also when using alt attribute, avoid using words that relates to "graphic" such as "logo" and others. An img is already an image/graphic so no need to describe it as one.
  • Using p tag as the toggle for the accordion is not really great/accessible. An interactive components uses interactive elements.
  • You could use button for the toggle but it requires some work since you will be needing to manipute the aria-expanded attribute, so a quick workaround is to use details element. This is a native element which is suited for accordions and is really accessible.
  • Dropdown-arrow should be hidden as well since it is decorative.

  Aside from those, great work on this one.

  Marked as helpful

  1

  ---

  

  Vicktor•900

  @Victor-NyagudiPosted about 3 years ago

  @pikamart Thank you for the feedback.

  I definitely considered using a button for the accordion, but I felt like the amount of styling that would go inside it would complicate things. I'll keep that accessibility info in mind either way.

  I also knew about min-height but never found the right place to use it. Thanks for the insight on that.

  1

  

  Vicktor•900

  @Victor-NyagudiPosted about 3 years ago

  Hey, @pikamart.

  Your feedback has been extremely helpful to me, so thank you once more!

  I went ahead and made the changes to my solution code, so if you'd like to see what it looks like now, feel free to check it out.

  I'm always looking to learn.

  Cheers!

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @Victor-Nyagudi Sure! Let me just see it again real quick.

  Looks great right now and it is accessible so really great by that.

  Also a suggestion if you would like. Right now clicking multiple accordion makes the layout bigger right, you can make a limit of the container's height and just have a scrollbar so that even if you click multiple items, the container won't be resize and you can just scroll inside the accordions if you like, you can do:

  .accordion__options {
    max-height: 290px;
    overflow-y: scroll;
  }
  

  I used that 290px because if I hover on the accordion container in dev tools, I get that height so I used it.

  Using this, the container won't be resized unlike before, but this creates a horizontal scrollbar, if you don't want the scrollbar but keep the functionality, you could use:

  .accordion__options::-webkit-scrollbar {
      display: none;
      -ms-overflow-style: none;
      scrollbar-width: none;
  }
  

  This way the scrollbar will be hidden. That is from w3schools. Though that is just for desktop, it is up to you for mobile on how you would implement the accordion:>

  1