testimonials-grid-section

Community feedback

• 

  Anton•515

  @antaryaPosted about 3 years ago

  Hi 👋,

  Great job, it looks really good.

  I have a couple of suggestions/improvements, though:

  HTML

  [1. Wrap your content with the main tag. That way browser will know where to find primary content. Also, it will fix some accessibility issues. Here is a resource with examples related to semantic tags.

  [2. Add alt attribute to img tag. It can be empty or should describe what is on the image. Screen reader users should have an idea of what is on the image based on the alt text. Here are some resources: mdn alt, great-alt-text.

  [3. HTML and CSS files are not formatted properly. It will make your code more readable if you use an editor extension that can format on each save for you. For example, if you are using vscode try prettier, for different editor search for editor-name prettier.

  CSS

  [4. Consider more universal approach for box-sizing using this method.

  [5. Take a look at this thread regarding different ways to name CSS colour variables, so it is easier to use while styling your app. slack discussion.

  [6. You are using :nth-of-type(N) pseudo-class a lot, which looks not very efficient. Next time you want to insert another card in the middle, you will have to change the rest; instead, you can create .card--bg-pink or .bg-pink and apply where necessary. Another example would be to have modifier .card--with-quote-bg, which will add quote background. That way, you make your code easier to read; and easier to apply changes. Here is a resource on how to structure your CSS using [BEM methodology] (http://getbem.com/introduction/), also later, you might be interested in this scss and this tailwindcss.

  [7. Where possible, try not to use fixed width and height so your elements can adapt to different screen sizes, limit using max-, min- version of width and height where required but allow to adapt. Also, it feels like not the best choice to use em, unless that was the intention to bond width to font size. One of the possible solutions using grid but without fixed width/height would be to remove all width/height except on containers and then have something like this:

  /* For this to work first in HTML, move current card#5 to be card#3 so it is ordered the way they appear. */
  
  .card {
    height: 100%;
  }
  @media(min-width: 1200px) {
    .container {
      padding: 20px;
    }
    .grid-wrapper {
      display: grid;
      grid-template-columns: repeat(4, 1fr);
      grid-gap: 20px;
    }
    .card:nth-of-type(1) {
      grid-column: 1 / 3;
      grid-row: 1;
    }
    .card:nth-of-type(2) {
      grid-column: 3;
      grid-row: 1;
    }
    .card:nth-of-type(3) {
      grid-column: 4;
      grid-row: 1 / 3;
    }
    .card:nth-of-type(4) {
      grid-column: 1;
      grid-row: 2;
    }
    .card:nth-of-type(5) {
      grid-column: 2 / 4;
      grid-row: 2;
    }
  }
  

  There are other ways to achieve this, e.g. using grid-template-areas.

  [8. It would look better if you move background-color to the body tag, so all area is filled with background.

  [9. Avatar size is not equal as it has width: 12%, so it depends on the size of the container. In this case, you might benefit from using fixed sizes as you know it should have the same size on all cards.

  .img-div {
    width: 45px;
    height: 45px;
  }
  

  Let me know if you have questions. I hope this will be helpful.

  Keep up the good work 🚀. Cheers!

  Marked as helpful

  0