Accessible Accordion with Pure JavaScript

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 10 months ago

  It looks OK for me, too. But I don't recommend setting a fixed width, that usually does more harm than good. So I would at least change width to max-width - this will allow it to shrink if needed.

  A few other things I noticed...

  HTML:

  • Headings must always be in order, so you should not jump from <h1> to <h3>. Hence, I would change all instances of <h3> into <h2>.

  • Footer text must be wrapped in a <p>.

  CSS:

  • You don't need to give the body a width of 100vw, as that is its default width anyway.

  • Similarly, the default root font size is 16px, so no need to declare it. Remember, though, font-size must never be in px. This is bad for accessibility, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead.

  • It is common to do mobile styles first, as its layout is usually less complex. This means setting min-width media queries, and they should be in rem.

  • You are giving main a box-sizing: border-box on smaller screens. box-sizing: border-box is usually set on all elements, like this:

  *,
  *::before,
  *::after {
    box-sizing: border-box;
  }
  

  This is usually done at the top of the stylesheet, as part of a CSS Reset. I would recommend including it every time - I use this CSS Reset by Andy Bell.

  Marked as helpful

  1

  ---

  

  Tenzin Choeda•210

  @tenze21Posted 10 months ago

  Is it ok to use the header tags like it is done here for styling purpose because i think the developer used the h3 tags to make the text a bit bold but the contents don't actually seems to be header. Just something that struck me while going through the code. @Islandstone89

  0

  

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 10 months ago

  @tenze21 No, you cannot skip headings. You can style it in CSS if needed, but the HTML document needs to have headings in order.

  0
• 

  Jack Smith•350

  @ClonephazePosted 10 months ago

  Solution looks good! I looked at the page, played around with it in responsive design mode at various sizes, and looked at your code for centering. Everything seems fine, on my screens everything is properly centered. Maybe a scrollbar was popping up and shifting things?

  My one recommendation would be to set your width to something more like "width min(350px, 90vw)". min() is a fun one, allowing contents to adjust for different conditions. It was always choose the smallest value to work with, in this case it would allow your main content to take up to 90 percent of the width of the screen but never appear smaller than 350px. Responsive widths like that are great for designing content that can adapt to various screen sizes without needing to specify widths at specific viewport sizes.

  Marked as helpful

  1

  ---

  

  Paulo Vitor Ferreira Gonçalves Pivotto•30

  @high-rollsPosted 10 months ago

  @Clonephaze thanks, that's a great idea to use the min function! I should learn more about the core functions in CSS, I only know a few of them.

  0