CSS Flexbox

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 3 years ago

  Following on from the feedback you requested 😊

  • it’s easier to review the html and styles separately, so try to put css in a separate file rather than having all in the document head
  • this only needs 2 heading levels. The paragraph that starts “30 day…” should not be a heading in my opinion. Both other headings “belong” to the original heading so should both be h2s. Remember heading levels are not about size they are about document structure, giving meaning to the content
  • On your questions about sections, it would indeed be invalid to have sections within sections. Sections actually don’t get announced by most assistive tech unless they are aria-labelledby a heading, so using them doesn’t add much value semantically at the moment. It’s fine how you have it.
  • the rest of the html looks good but I think it’s a real shame you’ve approached this challenge with flexbox when it is perfectly suited to css grid. That would get rid of the extra div (components-all)
  • always include a css reset at the start of your css to remove browser styles. That will make spacings like margins on headings and paragraphs much more consistent and controlled.
  • Never ever write font sizes in px. Use rem most of the time (occasionally em may be appropriate). This is important to ensure font always scales correctly when people have different zoom or text size settings. It’s actually good practice to use rem anywhere you might otherwise use px, so that everything scales together.
  • this is causing big problems: width: 50vw; (and when 80vw). You don’t need any width on that other than 100% which it should already be by default. It just needs a max width in px (or preferably rem).
  • on the same main element there is no need for position relative
  • on the same main element there so no need for the margins (maybe just some margin bottom to push away the attribution)
  • the body is missing it’s background color. Also it should have min-height: 100vh; and either flex or grid properties to vertically center content on the page. The body also needs a little padding, like 1.5rem to stop content hitting the screen edges (now we’ve removed the width from the component and used max-width)
  • grid doesn’t make sense as a class name for what you’re doing at the moment - should that class name be used on the main element perhaps? That would make more sense if it’s being used to create an actual grid.
  • padding should be the same on every box in this design so all boxes can have the same class
  • don’t use key words for font sizes. Again, that’s not controlled and consistent. Use rem.
  • it is better next time to work mobile first and use a min-width media query instead to override styles for desktop. That will almost always leave you with shorter and more performant code.
  • all the media query needs to do in this is change the grid template if you were using css grid, or change the flex, direction if using flex. In this challenge you can actually do it with no media queries at all I think but that’s unnecessarily hard and would potentially end up with even longer code. So don’t worry about using them (in your slack question) - just use when you need to it’s fine

  There you go - feedback as requested! Good luck

  Marked as helpful

  1