Responsive flex layout card

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted about 2 years ago

  Hi

  The preview site link isn't working on this? I can give feedback but it's waaaay easier if I can view in browser

  With HTML:

  • You've commented out the picture element. Why? That's exactly what you should be using here
  • Perfume should be in a paragraph tag and have letter spacing applied in css, not extra spaces in html
  • This card only has one heading element - the name of the perfume. Headings are the most important elements for giving your web document semantic structure (like any document, or a book with contents page - that's what headings are for). It's really important you don't misuse or misorder heading elements
  • the new price is not a heading
  • the old price needs wrapping in a <del> tag and some sr-only text adding before it to make it clear it is the old price, as screen readers are not told when text is styled with a line through it.
  • The attribution should be in a footer element

  With the CSS

  • Font size should never be in px - use rem
  • remove max width 1440px
  • did you mean to add a min-height of 100vh to the body? That's what will center it for you on desktop screens
  • Not sure why you have padding on the body and margin on main and 90% width on something (?) You only need one of these techniques if the purpose is to prevent the component hitting screen edges on smaller screens
  • Try to name classes better. _1 and _2 etc will be hard for your future self to understand
  • Min-width 376px is really really small to make a switch to the desktop layout! You should only be switching layout when there is room for it to change.

  Marked as helpful

  0

  ---

  

  Funsho Ayobanjo•150

  @ayobanjoPosted about 2 years ago

  @grace-snow Thank you very much for your response. Had a little bit of an issue on GitHub with the deployment when I tried to restructure the project folder. Its already working well now

  0

  

  Funsho Ayobanjo•150

  @ayobanjoPosted about 2 years ago

  @grace-snow Applied all your corrections. Thank you very much

  0
• 

  Hyron•5,870

  @hyrongennikePosted about 2 years ago

  Hi @ayobanjo,

  Nice job on your first attempt of the challenge

  You can remove the max-width: 1440px on the body by doing this the card will be in the middle of the page.

  The below is for mobile and will the stack the image and content.

  main.main > div {
      width: 100%;
  }
  
  main.main {
      flex-direction: column;
  }
  

  Hope this is helpful.

  Marked as helpful

  0
• 

  Lea•160

  @HatchinoPosted about 2 years ago

  Hi. First, the r of "perfume" is missing. It also lacks the letter-spacing property which allows you the spacing between lines for your paragraph. The icon is not the same as on the model. Your responsive is not adjusted between 440px and 374px (the price exceeds the card). Adding styling to the attribution is just my opinion.

  Marked as helpful

  0