Order price

Community feedback

• 

  IRVINE MESA•1,855

  @DrMESAZIMPosted over 2 years ago

  Hi @SebastienWeb

  This was a good attempt since its was your first challenge. My advice is to continue on that path by starting with newbie challenges.

  To make your solution look better here is my suggestion

  1. lets reduce the width of the card class on file css/main.css line 21 to 40%

  2 remove the overflow property for card class on file css/main.css line 25

  Marked as helpful

  2

  ---

  

  M.Panda•200

  @SebastienpandaPosted over 2 years ago

  @DrMESAZIM Thanks for your feedback! I put the overflow hidden property on the card so that it cuts the image so I don't need to put the border-radius on the image and the card and I will put 40% instead of 25%?

  1
• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hello, Sebastien! 👋

  It's great that you write the styling with the mobile-first approach however, adding a desktop styling which in this case, only changes the background image of the body element on 1440px is too late.

  Also, on my desktop view (around 1280px width) the card is trying to fill the entire width of my browser. So, I recommend adding some max-width to prevent this from happening.

  To wrap everything here are the points that I'm trying to suggest

  • First, the media query is only used for changing the background image.
  • Second, to make the card responsive you only need to set a max-width and that's it.

  I notice a div with role="banner" in the HTML which is actually the same as a header tag. I recommend making it as the body background image instead. header is used to wrap nav and logo element (not for the background image). I highly suggest reading the MDN documentation about role="banner".

  There's no need to use role attribute. Why would you use it?

  1

  ---

  

  M.Panda•200

  @SebastienpandaPosted over 2 years ago

  @vanzasetia Hello, I'm going to correct this and for the attributes because in the html errors that the site sent me, it told me that the roles were missing or I did not understand everything

  1

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @SebastienWeb What is the site that told you about role attributes? The Frontend Mentor report doesn't say anything about role attributes.

  I highly recommend removing the div for the background image. You can use CSS background properties on the body element.

  Also, remove the role attribute from the main element. The purpose of role attribute is to make the non-semantic HTML like div and span to work as semantic HTML. However, it's best to always use to native HTML element whenever possible.

  0

  

  M.Panda•200

  @SebastienpandaPosted over 2 years ago

  @vanzasetia The site is mdn

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @SebastienWeb Where did you get the error notification?

  0

  

  M.Panda•200

  @SebastienpandaPosted over 2 years ago

  @vanzasetia view report

  0