HTML,CSS,FlexBox, Grid

Community feedback

• 

  Stephen Funderburk•180

  @Mr-FunderburkPosted about 1 year ago

  Nice job!

  There are two different images we were supposed to use for mobile/desktop; though I don't think your solution is a bad one. The idea behind using two different images is to ensure the images load quickly on mobile devices. For me, I found it easiest to create a div the size of the image and set the background to be the image for desktop/mobile with media queries.

  It's my understanding that we are trying to get our solution to look like the design that is given to us. Therefore, I would spend a bit more time on your CSS.

  • There's an icon missing on the button -- in fact, this should have been a styled anchor tag since we're not using a form here.
  • The typeface and font size of the "new" price should be changed to match the display
  • The spacing you have is a lot larger than the display, however, I wouldn't say that would "lose points" on this. I do think too much space creates a disconnect; so I would bring it in a bit.
  • As you've seen from the report that is generated when you submit, you're missing semantic tags (header, main, etc.). I don't fully agree with the standards because these are just snippets of what would be a larger project. However, I know that will be brought up.
  • I wouldn't say it's strictly wrong to put everything in a div/container, however, it will create a lot of elements that you don't necessarily need. The image for example doesn't need a container. You could have changed its display and treated it as a block element without the use of a div. The class 'product-info' could have been left off and just targeted the h4/'title' class. Instead of a div for 'product-description', this could have been a paragraph tag, and the span isn't needed.

  Overall, I would say great work!

  1