3-column-preview-card-component

Community feedback

• 

  Ahmed Bayoumi•6,740

  @Bayoumi-devPosted over 2 years ago

  Hey Nada, It looks good!...

  My suggestions:

  • All page content should be contained by landmarks, Contain the footer with <footer> instead of <div>.

  <footer class="footer">
        //...
  </footer>
  

  • Using more than one <h1> is allowed by the HTML specification, but is not considered a best practice. Using only one <h1> is beneficial for screenreader users.

  ---> Multiple <h1> elements on one page

  • A better way to make border-radius for the element... Add border-radius: 15px; and overflow:hidden; to Cards Container:

  .container {
      max-width: calc(300px * 3);      /* <--- Add */
      display: flex;
      flex-direction: row;
      justify-content: center;
      border-radius: 15px;      /* <--- Changed */
      overflow:hidden;      /* <--- Add */
  }
  .cards {
      /* width:300px;     <--- Remove */
      /* height: 500px;     <--- Remove */
      padding: 40px;
  }
  .card-1 {
      background-color: var(--Bright-orange);
      /* border-top-left-radius: 15px;     <--- Remove */
      /* border-bottom-left-radius: 15px;     <--- Remove */
  }
  .card-3 {
      background-color: var(--Very-dark-cyan);
      /* border-top-right-radius: 15px;     <--- Remove */
      /* border-bottom-right-radius: 15px;     <--- Remove */
  }
  @media(max-width:768px) {      /* <--- Changed */
      //...
      .container {
          max-width: 300px;      /* <--- Add */
          align-items: center;
          flex-direction: column;
      }
      .card-1 {
        /*  border-bottom-left-radius: 0;     <--- Remove */
        /*  border-top-right-radius:15px;     <--- Remove */
        /* border-top-left-radius:15px;     <--- Remove */
      }
      .card-3 {
      /* border-top-right-radius: 0;     <--- Remove */
      /* border-bottom-right-radius:15px;     <--- Remove */
      /* border-bottom-left-radius:15px;     <--- Remove */
      }
       //...
  }
  

  Hope this is helpful to you... Keep coding👍

  Marked as helpful

  0

  ---

  

  Nada El-shater•70

  @Nada-El-shaterPosted over 2 years ago

  @Bayoumi-dev Thank you very much Ahmed for your effort, it was very useful and I learned something new. I hope to hear any feedback from you again soon <3

  0

  

  Ahmed Bayoumi•6,740

  @Bayoumi-devPosted over 2 years ago

  @Nada-El-shater You are welcome😊

  0
• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi, Nada! 👋

  You can add overflow: hidden to the container element. By doing this, you can remove all the border-radius properties from all cards.

  Some feedback.

  • I recommend changing all the h1 to h2. It's because h1 is used as an identifier for each. So, it should be only one and unique for each page. In this case, it's okay to not have h1 since it's not a full website.
  • It's great that you specify the type of each button element but I would expect those buttons as links. As a user, I think it should navigate me to another page if I click one of those buttons.
  • I am not sure why some of the car icons have different colors.
  • For the font family, I recommend only importing the necessary font-weight. Currently, it is importing all the font weights. Also, you can import all the necessary font families with one @import rule.
  • Remove the width and the height properties from the .cards styling. For the width, just set a max-width on the container to prevent the card from becoming too large. For the height, let the elements inside the card control it.
  • Make sure to use either px or rem (recommended) for the max-width. If you set the max-width with a percentage unit then it will always grow based on the user's viewport width.

  That's it! Hope this helps.

  0