Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Responsive FAQ accrodion

Binyam 400

@binzam

Desktop design screenshot for the FAQ accordion coding challenge

This is a solution for...

  • HTML
  • CSS
  • JS
1newbie
View challenge

Design comparison


SolutionDesign

Community feedback

T
Grace 30,010

@grace-snow

Posted

Hi, I hope this feedback is helpful

  1. Header is a very important landmark. It cannot be included and then have no content inside! It needs to be removed
  2. It's not recommended to add empty divs to html purely for styling reasons. If the image is changing between screen sizes it would be much better to use the picture element for good performance. If the image never changes then background image is OK
  3. The star image is decorative so should have empty alt. So is the plus you have incorrectly labelled as "icon"
  4. Headings must be in order. This is extremely important for accessibility and SEO. You cannot jump from a h1 to h4
  5. It is essential to use the correct elements for interactivity. Heading elements are not clickable so this is inaccessible. Accordions like this are called "disclosures" — Read this to find out what you need
  6. It is more performant to link the fonts in the html head instead of css imports
  7. Css should always start with a modern css reset at the start of the styles in every project
  8. Font size must never ever be in px
  9. Max width on the component should be in rem not px. You will want to have control of its max width and shouldn't need to change this at different screen sizes. With % you have no control and are forced to change max widths often.
  10. It is very confusing naming classes the same as established html landmarks when they are used elsewhere. At least use something like "faq-header" instead of just "header" so it's less confusing reading the css
  11. Use padding on the component not max width in %
  12. Media queries must be defined in rem or em for responsive sites. You only need one media query in this challenge. Once the above is fixed you should be able to delete others.
  13. Once the html is fixed you will need to change the js a little as it will need to toggle aria- state. You will be able to do most of what you are doing now all in the css instead of js.

Marked as helpful

0

Binyam 400

@binzam

Posted

@grace-snow Thank you for the feedback, It is very helpful!

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord