Skip to content
Submitted about 3 years ago

HTML, CSS - GRID - FLEX, JS

Mathias de la Fuente•40
@mathiconh
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

Solution retrospective

Any feedback would be appreciated!!!

Community feedback

  • Vanza Setia•27,775
    @vanzasetiaPosted about 3 years ago

    👋 Hi Mathias!

    👍Good job on completing this challenge! This challenge has a tricky position for the images 😅.

    I have some feedbacks on this solution:

    • Wrap all your page content with main tag except the attribution. For the attribution, swap the div with footer.
    • Currently the accordions are not accessible using keyboard (toggle the accordion panel) and screen reader. I recommend to use summary and details tags instead of div. For the JavaScript, you're going to only allow the user to open one accordion panel at a time (like what you have done).
    • Also don't forget to add :focus-visible style to all accordion panel. This is helpful for users that navigate your website using keyboard.
    • For 0 value on CSS, you don't need to put any units after it. Just write 0.
    • Use rem or sometimes em unit instead of px. Using px will not allow the users to control the size of the page based on their needs.
    • Don't limit the height of the body element, it will not allow the users to scroll the page if the page content needs more height. Use min-height instead.
    • Make sure that your solution looks good on all screen sizes, currently on mobile landscape mode and on desktop view doesn't look good.
    • About the container, I guess you can don't need to set any height for it, I recommend to let the padding and the elements inside it that control the height. This will make your container more responsive.
    • Remove this CSS code unless you have a reason of doing it. If you have the reason, please tell me.
    html {
  height: 100%;
}
    • Consider using js- as the prefix for any DOM elements that you want to manipulate through JavaScript and don't put any styling to any js- class (it's only for JavaScript). This will make your code more maintainable!
    • I recommend to use let instead of var to any variables that you want to reassign.

    That's it! Hopefully this is helpful!

    0
    Mathias de la Fuente•40
    @mathiconhPosted about 3 years ago

    @vanzasetia wow, i appreciate your feedback, really u help me so much. Before to continuing, i need say "I don't know speak english, but i try, sorry".

    For this code: html { height: 100%; } i have complications with the backgroung color, this color does not fit full screen, so, i add this code in <html> and this is solved.

    Really thanks for comment my code and helpme to grow. I will try to apply the changes mentioned above.

    0
    Vanza Setia•27,775
    @vanzasetiaPosted about 3 years ago

    @mathias9968 About the height property on the html element, I got it. But, I would recommend to set min-height: 100vh; on body element instead on the html element. By setting the min-height: 100vh on the body element, you can remove the background-repeat property and the height property on the html element.

    You're welcome! 😉 Glad that it is helpful for you!

    0
    Mathias de la Fuente•40
    @mathiconhPosted about 3 years ago

    @vanzasetia Yes, background-repeat is not doing anything here. take a new screenshot with a more similar design, did you see it?. Thank you for your recommendations, I appreciate so much.

    Have a nice day dude!! :) Greetings from Argentina.

    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