Skip to content
OverviewSolutions
4
Comments
4
LEUILLIER
@JulienLEUILLIER

All comments

  • LEUILLIER•120
    @JulienLEUILLIERPosted about 3 years ago

    Your code works, you just need to add height to .accordion-svg. By default backgrounds take the height of their container if I'm not mistaken, and if you use your browser Dev-tools you'll see the container has no height. Hope that helps !

    Marked as helpful
    1
  • Jesse Ma•95
    @jma26Submitted about 3 years ago

    This challenge was a bit more difficult compared to the earlier challenges. I had trouble figuring out how to implement the desktop images with the bg-pattern and did not realize I could do this with the help of background-image, background-position etc. (seems like I'm not thinking about the css background property and defaulting to using img element). In addition, I had an issue dealing with the parent height jumping as the child element (accordion) expands and collapses. For now, I used a fixed height on the parent but am curious how this issue can be achieved or fixed.

    Tried to make sure the accordion complies with ADA Web Accessibility and is toggleable with a keyboard etc.

    Any thoughts or opinions regarding the height issue and 1st try on ADA Web Accessibility is highly appreciated!

    Thank you!

    #HappyCoding

    FAQ Accordion - HTML, CSS, JavaScript

    1
    LEUILLIER•120
    @JulienLEUILLIERPosted about 3 years ago

    Hi Jesse ! I did this challenge a few days ago, nice work on this one. I'm not an expert by any means but I've got some feedback on this one :

    • You should make it so the arrow is a background to the button, thus making it clickable to expand the question.
    • For the expanding animations, I used this approach. But I like your use of height: auto with visibility.
    • I don't think a fixed height is really an issue, I used a dynamic height but it's really preference I guess ? With animations on the expansion the height changes are more smooth.

    Great job !

    Marked as helpful
    1
  • LEUILLIER•120
    @JulienLEUILLIERPosted about 3 years ago

    Good work !

    I would allow users to click on the whole .line element to see the answer. I think you're also missing the mobile background image on mobile layout! Keep up the good work!

    Marked as helpful
    0
  • LEUILLIER•120
    @JulienLEUILLIERPosted about 3 years ago

    Really nice solution, with responsiveness on the mobile layout !

    You can use classList.toggle to actually add and remove the .active class in your script, to save a few lines.

    Keep up the good job !

    Marked as helpful
    1