repsonsive product card using CSS Flexbox

Community feedback

• 

  hitmorecode•6,680

  @hitmorecodePosted about 1 year ago

  In general your page looks good. I see you used three media queries. You could have done it with just one

  • When creating a page, go with mobile first approach. This is actually the default setup of a page.
  • When you have a class inside another class, you don't have to write it like that in css. For example you have class .previous-price and it's inside .product-price, you don't have to do it like this

  .product-price .previous-price {
    font-family: 0.2em;
    text-decoration: line-through;
    color: var(--color-dark-grayish-blue);
  }
  
  ## You could have done it like this, because previous-price is your target and css knows where it is.
  .previous-price {
    font-family: 0.2em;
    text-decoration: line-through;
    color: var(--color-dark-grayish-blue);
  }
  

  If you have something like this

  <div class="main">
    <p>Lorem ipsum lorem ipsum</p>
  </div>
  
  <div class="second-main">
    <p>Lorem ipsum lorem ipsum</p>
  </div>
  

  Because the p tags have no class and if you want to target a specific p tag, you cannot tell css to target a p, it will target all p tags. In this case you can do it like this

  .main p {
    color: green;
  }
  
  .second-main p {
    color: yellow;
  }
  

  If you give a p tag a class or id, then you can just target that class or id

  You usde margin to place the card in the middle of the page, you don't have to user margin, use flexbox or grid.

  ## Lines that are commented out can be removed.
  body {
  /*   padding-inline: 1rem; */
    display: flex; /* add these lines */
    min-height: 100vh;
    justify-content: center;
    align-items: center;
    flex-direction: column;
  }
  
  .product-card {
    max-width: 65rem;
    height: 45rem;
    /* aspect-ratio: 4 / 3; */
  /*   margin: 6rem auto 2rem; */
    background-color: var(--color-white);
    box-shadow: 2px 2px 5px gray;
    display: flex;
    border-radius: 1rem;
    overflow: hidden;
  }
  

  Marked as helpful

  2

  ---

  

  Kalab Sisay•10

  @KalabSisaySEPosted about 1 year ago

  @hitmorecode Thanks for the feedback. You are right It is unnecessary to write nested selectors when an element can simply selected by its unique class. It's just that I am used to writing selectors this way cause it helps me get used to the location of the element i.e. the HTML structure faster.

  I am going to use the mobile-first approach and simple selectors in the future.

  0
• 

  Jennifer Souza•340

  @zoedarkweatherPosted about 1 year ago

  Hi, looks pretty good overall. I agree with the above comment that mobile-first is the way! I'd also point out you can use the picture element in your html to switch out the image for the correct screen size. Here's the mdn reference page on that if it might help: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture. Best!

  Marked as helpful

  1

  ---

  

  Kalab Sisay•10

  @KalabSisaySEPosted about 1 year ago

  @zoedarkweather Thanks for the feedback. I will start to use the <picture> element in the future

  0