HTML, CSS - GRID - FLEX, JS

Community feedback

• 

  Vanza Setia•27,715

  @vanzasetia

  Posted over 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!

  03