Skip to content
OverviewSolutions
0
Comments
2
SB
@stueybrock

All comments

  • Ryan1731•10
    @Ryan1731Submitted 10 months ago

    Thank you for taking interest in my first ever submitted frontend mentor project. What I found difficult: Animating the opening/closing effect, making the site fully responsive, adding keyboard navigation. Question: I would appreciate any help in making the site fully responsive for devices less than 400px.

    FAQ Accordion

    1
    SB•40
    @stueybrockPosted 10 months ago

    Hey,

    First of all, great job, it's looking pretty similar to the designs.

    In terms of code feedback:

    • With this type of project, I would always firstly focus on the HTML before touching anything else. You're building an accordion here with clickable sections but semantically you're using divs. To make it accessible, you'll want to use something like a <button>. I would strongly recommend search for accessible accordion examples to get some solid HTML markup, that includes all of the accessibility attributes you'll need such as aria-expanded, aria-controls etc.
    • You're getting an issue with height on your smallest screens because you're hard-coding a max-height in your Javascript, which is not unset for smaller screens. As a rule of thumb, I would always separate my concerns, so CSS is done CSS files, Javascript is done in Javascript files etc.
    • I don't see a reason why you'd separate your media queries into a different file. It's easier to maintain and review code if all of the CSS is in one place. Splitting up CSS into separate components is good too!
    • It's a little bit jumpy on open and close, worth investigating that further.
    Marked as helpful
    1
  • SB•40
    @stueybrockPosted over 2 years ago
    • Consider changing name of class 'card'. What you've called 'card' is actually the wrapper of multiple cards, so might make more sense to give that a class of 'card-wrapper' for instance and what you've called 'card-col' to just be 'card'.
    • Perhaps an <article> would be better semantically than a <section> for each card.
    • Best practice is to have one <h1> per page, for SEO and accessibility. I would change to a <h2> in this instance.
    • Use an <a> instead of your <input> for the Learn more link – it's better semantically.
    • By setting the image as a background image, you are giving yourself less control over the size and and removing the ability to add alt text for assisstive technologies. I would just have either an inline SVG or an <img> tag.
    • You've set a class of 'card-col-text' but in the styles you've directly targetted the p instead of the class. If you're doing that because the styles were getting overwritten, then you've got a specificity issue and consider restructuring the CSS.
    • Different amount of body text in the <p> changes the width of a section. Is that intentional?
    Marked as helpful
    2