Skip to content
Submitted almost 2 years ago

FAQ Accordion Card

Sree Mouli•60
@SreeMouliChinta
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...FAQ accordion card
  • HTML
  • CSS
  • JS
1newbie

Design comparison

SolutionDesign

Community feedback

  • Jeff Guleserian•480
    @jguleserianPosted almost 2 years ago

    Good morning, Sree!

    Thank you for submitting your solution to the Accordion Card FAQ Challenge. I enjoyed looking through your code - there was so much to admire. I like your reset of rem and the responsiveness you built into your work. Your JavaScript was easy to read and follow and efficient. In fact, I could spend a great deal of time running through what I liked, but I know what you probably want/need are some more critical observations. Here are my contributions to that respect. Keep in mind that this is only the opinion of a novice, not an expert.

    Accessibility:

    • While “FAQ” is certainly the largest headline of the page, if you think about it, it’s really the subtitle to something else, like “Membership Subscription” or something similar. A person using a screen reader will hear “FAQ” as being the primary subject and may not understand that this FAQ has to do with “membership” and not something else. As a matter of best practice, then, it might be a good idea to put an <h1> at the top of the body with the main subject title and leave “FAQ” as an <h2>. The <h1> could have a class=”sr-only” (or similar) in which {display: none} appears in the CSS. This will make it invisible and take it out of the flow, but the screen reader will still read it.

    HTML:

    • As you probably saw in the warning you were given on your solutions page, it would be a better idea to put your img.box-img in a container such as a <section> or <div>. Not only will this give you an extra layer of control over positioning, but also grouping with other objects and visibility (as is the case here). Overflow visibility has to be controlled by the container in which something sits. You won't have this option if your <img> is not in a container of some sort.
    • You may want to take a look at the <picture> element instead of using <img> all the time. The advantage with the <picture> elements is that you can set all your pictures in one html tag and trigger them to appear at different breakpoints. Here is a good explanation on W3Schools: ![W3Schools: <picture>] (https://www.w3schools.com/tags/tag_picture.asp) This saves a lot of style sheet additions in the media queries and turning on/off display properties.
    • Another tag you may want to take a look at is <details>. ![W3Schools: <details>] (https://www.w3schools.com/tags/tag_details.asp). What I really like about the tag is that is has the expandable accordion already built in. It uses <details> as the main tag, <summary> for the heading, and a <p> for the explanatory text. It even has an animated caret (bullet point) to the left of the <summary> tag plus a click event already built in. All you have to do is change the styles. Then, to move the caret to the right hand side, simply change the list-style property to “none.” To use the icon as your caret, just insert it as a content property ::after the element and style it accordingly. Anyway, it could save you some time and coding in the future. Here is how I did min, in case you're interested:
        font-weight: 400;
    font-size: 1.4rem;
    line-height: 1.9rem;
    color: var(--blue-dark);
    list-style: none;
}

details>summary:hover {
    color: var(--orange);
}

details>summary::after {
    display: inline;
    content: url("/images/icon-arrow-down.svg");
    float: right;
}

details[open]>summary::after {
    transform: rotate(180deg);
}

details[open]>summary {
    font-weight: 700;
    color: var(--blue-dark);
    cursor: pointer;
}

    CSS

    • Media query – I think you may have had a typo in your CSS @media. You set the max-width property with “em” instead of “rem.” The rem is more reliable and easy to calculate (10px/rem), since it goes back to the <html> setting. I also wondered if you also meant to write 37.5rem instead of 60em. At 60em, the pixels are counted (in situ) in the <body> at 1.2rem (according to your stylesheet's <body>) or 12px. This would make the width of the viewport change at 720px. The reason I ask is that your breakpoint is happening at too wide of a viewport. Although it still looks good, it wasn’t what the challenge asked for. You may have done this on purpose because, frankly, I think it needed a wider breakpoint.
    • Measurements: I noticed some of your measurements are a bit off. I have a PRO subscription to Frontend Mentor, so I have access to the Figma file which gives much better detail. You may not have this luxury. No worries, I’ll just point out what I see, and you can decide its importance.
      • section.container needs a wider left margin – I know it looks “off” like that, but I’m sure it’s to balance the overflow of the box image.
      • <h1> font is too small. This is also causing your expandable list to be off
      • The placement of each “half” of the graphic is off. This is probably because you set your grid-template-column with “1fr 1fr”. While this makes the columns beautifully even, I don’t think they are supposed to be. They should be more like 42% / 58%.
      • I think you tried to compensate for the uneven column by using padding on the second column. Eventually, this caused your FAQ section to be wider than the model. Again, if you don’t have the Figma file, this would have been more difficult.

    JavaScript

    Your JavaScript is very comprehensible, easy to read and follow, and it works! It looks really great! My only question is why you didn't use ES6 syntax, but that's not a huge issue.

    Well, Sree, again, great job completing the challenge. I hope my comments are helpful. Just take them or leave them as you see fit. I hope to see you around again soon. Thank you for letting me learn from your coding. I appreciate it very much.

    Jeff

    Marked as helpful
    0

Please log in to post a comment

Log in with GitHub

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