Product review card html and css solution

Community feedback

• 

  Brian Schooler•440

  @superschoolerPosted over 2 years ago

  Hi Rick, great to see you're starting to get into web development - this site is a great resource I wish I found years ago when I was dabbling in it.

  While your card looks great in the mobile view, once it changes to desktop the responsiveness isn't great. One quick change you can make to fix it is adding the following to the CSS:

  .desktopImage { max-height: 60vh; }

  This will keep the image from reaching 100+ viewport height and overruling the "center" div you have. One more small thing is that the naming convention for html should be "desktop-image" rather than "desktopImage". The latter is called camelCase and should be reserved for JavaScriopt.

  Aside from that, I'd change the breakpoint to a minimum of 700px as anything smaller compresses the text too much. It's OK to keep the mobile view as wide as you need to until the desktop version looks good.

  Marked as helpful

  1

  ---

  

  João Gabriel•340

  @joaojgabrielPosted over 2 years ago

  @superschooler, .desktopImage { max-height: 60vh; } actually breaks it even further. What happens is that the height of the image itself changes (to stay at whatever is the current 60% of viewport height), but the card height doesn't move along with it. That happens because the content at the right is made up of a bunch of separate elements in normal flow:

  <div class="center">
          
          <p class="perfume">perfume</p>
  
          <h1>Gabrielle Essence Eau De Parfum</h1>
  
          <p>A floral, solar and voluptuous interpretation composed by Olivier Polge, 
          Perfumer-Creator for the House of CHANEL.</p>
          
          <div class="flex">
            <p class="newCost">$149.99</p>
            <p class="oldCost">$169.99</p>
          </div>      
                
          <button class="button"><span><img src="images/icon-cart.svg" alt="cartIcon"></span>Add to Cart</button>
          
          <div class="attribution">
            Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>.<br>
            Coded by <a href="https://www.frontendmentor.io/profile/joyrexjrl" target="_blank">Rick Lozano</a>.
          </div>
  </div>
  

  And the default behavior in normal flow is for content that has a bigger height (or block-size to be more precise) than their enclosing box, or parent element, to overflow.

  Flexbox and Grid deal with unwanted overflow. So what could be done for the content (and the card follows) to be responsive is making it a Grid with only rows or a Flexbox with flex-direction: column. In my solution, I did the latter (you can see it live here). Here's the relevant code:

  The HTML of the whole element:

          <div class="content">
            <div class="description">
              <span class="type">Perfume</span>
              <header class="name">
                <h2>Gabrielle Essence Eau De Parfum</h2>
              </header>
              <p>
                A floral, solar and voluptuous interpretation composed by Olivier
                Polge, Perfumer-Creator for the House of CHANEL.
              </p>
            </div>
            <div class="prices">
              <span class="actual"> $149.99 </span>
              <span class="original"> $169.99 </span>
            </div>
            <a href="">Add to Cart</a>
          </div>
  

  And the CSS to make it a Flexbox:

  .content {
    display: flex;
    flex-flow: column;
    justify-content: space-between;
  }
  

  Phew... This comment took me a while. I hope it helps someone. :) Btw, I agree with the point of making the breakpoint close to 700px, Brian.

  Marked as helpful

  1
• 

  Rick•270

  @joyrexjrlPosted over 2 years ago

  awesome thank you both, I have gone through and made adjustments to the naming conventions to try and follow some standard. I am use to working with c# and unity (in a very beginner state) so my naming of things comes from that. I have also centered the card using the absolute positioning method, and changed the px count closer to 700. This will help me a lot with the next projects I start working on. Thank you both. I have updated the github, not sure if the "site" has updated or not. Thanks again

  0