Responsive card product, using flex and grid.

Community feedback

• 

  juss-10•40

  @juss-10Posted 10 months ago

  Hello!

  Congrats on finishing the challenge! Visually the component looks very close to the design, but there are a number of issues in the code.

  Here are some problems I noticed:

  Height and Grid

  Using 100% in body { grid-template-rows: 100% 1fr } occupies 100% of the height set by body { height: 100vh } for the first grid row, which does not leave any additional space for 1fr in the second grid row. This is causing the text in <div class="attribution"> to overlap with the product card.

  • You can change body { height: 100vh } to body { min-height: 100vh } as well as change body { grid-template-rows: 100% 1fr } to body { grid-template-rows: 100% auto } or body { grid-template-rows: 1fr auto }.

  • Setting body { min-height: 100vh } can have some issues on mobile, but there are other ways of approaching setting a height for body. I recommend you look more into this.

  Use <picture>

  2. I would not recommend using two separate img elements and hiding one or the other when needed. Instead, you can use the <picture> and <source> elements along with the img element and an inline media query with the media HTML attribute on the <source> elements to specify which image to use when the width of the viewport is at that size:

  <picture>
  	<source media="(min-width: 60rem)" srcset="images/large-image.png">
  	<source media="(min-width: 40rem)" srcset="images/medium-image.png">
  	<img src="images/small-image" alt="Description">
  </picture>
  

  Capitalize text in CSS

  Instead of capitalizing text in the HTML, use CSS to do this.

  selector {
  	text-transform: uppercase;
  }
  

  Price elements

  The price does not need to be a heading and the structure can be simplified a bit. There are few other adjustments here that you can make. - The visually-hidden class will visually hide Old price: for sighted users and allow users with screen readers to hear this text. You can find out more about using a visually-hidden class online.

  <p class="price">
  	<span>$149.99</span>
  	<s><span class="visually-hidden">Old price: </span>$169.99</s>
  </p>
  

  Buttons

  I'm not sure why you used a div element for the button, but that's not recommended. That is not a button. Use a <button> element for "actions" and use <a> for links. You sometimes may have links (<a>) that look like buttons, but still only use <button> for actions and <a> for links.

  Decorative images and icons

  The shopping cart icon in the button is decorative, so it doesn't need to include a value for alt text.

  Product Card Container

  Personally, I would not have used <main> as the container for the product card. I would use its own container. This might be <article>, possibly <section>, or just <div>.

  I hope this helps!

  0

  ---

  

  Nantue Polchevski•170

  @NantuePosted 10 months ago

  hi @juss-10, Thank you for your recommendations, they really help me a lot.

  I don't understand very well about changing main for article, section or div.

  I was already using div, but they had told me to change it to main haha. I had been putting role: "main" in the <div/> so that I wouldn't get the error in frontendmentor.

  0
• 

  Nantue Polchevski•170

  @NantuePosted 10 months ago

  Almost perfect

  0