My submission for Accordion card

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted about 3 years ago

  👋 Hi Quang Hung Pham!

  I have some feedback in this solution:

  • On mobile view the card is too narrow (360px * 640px). Remove the margin from the card. I recommend to use only padding on body element to prevent the card having full width on small screen devices.
  • To make the card position right in the middle of the page try to apply this styling.

  body {
    display: flex;
    flex-direction: colum; /* To prevent it become two columns */
    align-items: center;
    justify-content: center;
  }
  

  • I recommend to only allow the user open one accordion panel at a time, since if the user open all the accordion panels at the same time, it causing the card have too much height, which doesn't look good in my opinion.
  • The outline color for :focus-visible style is too similar with the background-color, I recommend to use darker color like this one rgb(11, 11, 11).
  • Also, the color of the accordion-question text doesn't match to the design. Try to find and use the right color.
  • In my opinion, you should give your variable name in JavaScript with more meaningful name. Like for example, instead of item as the name of the current element, why don't use button instead? It's more clear and help yourself understand the JavaScript code in the future.

  That's it! Hopefully this is helpful!

  Marked as helpful

  0
• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @pqhung3007 👋🏻

  I think I have a quick fix for the accessibility and HTML issues.

  • In the code, <section class="card">...<section> should be <main class="card">...<main>. This will fix the most of the accessibility issues.
  • Next, <img src="images/illustration-woman-online-desktop.svg" alt="" srcset=""> here you need to remove srcset and add description in alt. And this will help to fix the HTML issues, but don't forget to generate a new report after you fix the issues.

  I hope this was helpful 👨🏻‍💻 other than those issues, everything else looks good. Cheers 👾

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted about 3 years ago

  @kens-visuals I agree with all things that you said, except " add description in alt".

  All the images in this case are decorative images. The reason for that is because they don't at meaning or information to the user and also if the images don't exist, the user still can understand what's going on the page.

  So @pqhung3007 what you have to do instead is to leave the alt="" empty and add aria-hidden="true on every img tag.

  0
• 

  Quang Hung Pham•470

  @pqhung3007Posted about 3 years ago

  Thank you for such a detailed feedback! I'll try to update some of things that you recommended.

  0
• 

  Quang Hung Pham•470

  @pqhung3007Posted about 3 years ago

  I haven't paid much attention to those small attributes and tag name until now and your comments really make me think again. Really appreciated!

  0