Just HTML & CSS

Community feedback

• 

  Anton•515

  @antaryaPosted over 3 years ago

  Hi,

  Looks cool. Nice job.

  I have a couple of suggestions, though:

  HTML

  1. Semantic tags are misused. You use them to describe the page overall structure, not the structure of a specific component (card). Here is a link that might be helpful https://learn-the-web.algonquindesign.ca/topics/html-semantics-cheat-sheet/. So in your case, your page has no header and footer, so that you can use div instead; your main tag should consist of the primary content of your page, e.g.:

  <main>
    <div class="card">
      ...
    </div>
  </main>
  

  2. alt attribute 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. In this case, you might leave it empty, which means the screen reader will skip it because you already have the person's name, and that should be enough. https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/alt

  CSS

  3. If you use reset styles, you will eliminate some differences across browsers or remove unnecessary spacing. You can read more about reset/normalize here https://css-tricks.com/reboot-resets-reasoning/. Also, it is helpful to include https://css-tricks.com/box-sizing/ in your reset.

  4. .card-img can also be formated using BEM (http://getbem.com/introduction/) like .card__img or .card__avatar.

  5. You will have trouble using fixed width and height for sections; you want all elements to adapt to viewport changes. So instead of fixed width/height on box1, .card__header, and card__body remove and add this:

  .card {
    ...
    width: 100%;
    max-width: 348px; /* can be less but no more than 348px */
    ...
  }
  

  6. It looks like you had trouble applying border-radius to the top section, so you used direct styles. There is nothing wrong with applying border-radius to each corner, but there is a simpler solution. You need to add overflow: hidden; to the .card class and clean the rest.

  .card { /* box1 in your case */
    ...
    border-radius: 10px;
    overflow: hidden;
    ...
  }
  

  7. One simpler solution to position avatar is instead of using bottom everywhere, remove it and use margin-top: -54px;, and clean unnecessary margins, and position: relative; from .card__header.

  8. For avatar define fixed width and height, so it is predictable. In case you have a different image with no square dimensions, it will look strange.

  .card__avatar {
      display: block;
      width: 100px;
      height: 100px;
      border: 4px solid #fff;
      border-radius: 50%;
      background: #fff;
      /*
        position: relative;
        bottom: 52px;
      */
      margin-top: -54px; /* shift image */
      object-fit: cover; /* https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit */
  }
  

  9. There are multiple ways to centre elements vertically and horizontally. You can check other solutions, or you can do something like this:

  /* assuming there is a reset */
  html, body {
    height: 100%;
  }
  main {
    display: flex;
    align-items: center;
    justify-content: center;
    margin: 0 auto; /* center main tag horizontally as we restricted width */
    padding: 20px;
    min-height: 100%; /* this will make sure main will at least occupy viewport height */
  }
  

  10. You can use the shorthand of margin property https://developer.mozilla.org/en-US/docs/Web/CSS/margin

    /* top | right | bottom | left */
    margin: 30px 0 0 0;
  

  11. Instead of px, use rem units. Here is a link that might be helpful https://www.sitepoint.com/understanding-and-using-rem-units-in-css/

  I hope this will be helpful.

  Keep up the good work. Cheers!

  ---

  Browser Version: 92.0.4515.107 (Official Build) (64-bit) OS: Ubuntu 20.04

  Marked as helpful

  2

  ---

  

  Fernando•145

  @Faerk77Posted over 3 years ago

  @antarya Thank you so much! i will keep an eye on this suggestions.

  0