Profile card component built using HTML and CSS

Community feedback

• 

  Yazdun•1,310

  @YazdunPosted about 3 years ago

  Hello Lucy and well done on the challenge, Here are my suggestions :

  • Use Prettier extension on your IDE to format your code.

  • use code below instead of specifying default value for html element separately

  *,
  *::before,
  *::after {
    margin: 0;
    padding: 0;
    box-sizing: border-box;
  }
  

  • Html elements are block level by default ( thats one of the reason for approaching mobile first ), so you don't need 👇

  display: flex;
      justify-content: center;
      align-items: center;
      flex-direction: column;
  

  on your main tag, you get the column by default

  • give min-height:100vh instead of height:100% to your body.

  • right now, background circles are not responsive and layout breaks quite easily on smaller screens, so here is what I did on this challenge for background circle to make sure of their responsiveness. firstly add 👇

  .c1 {
    right: 50%;
    bottom: 43%;
  }
  
  .c2 {
    top: 50%;
    left: 50%;
  }
  
  .c1,
  .c2 {
    position: fixed;
    transition: 0.5s cubic-bezier(0.075, 0.82, 0.165, 0.6);
    z-index: -1;
  }
  

  to your css and then add images to the markup 👇

    <img
          src="./assets/images/bg-pattern-top.svg"
          alt=""
          role="presentation"
          class="c1"
        />
        <img
          src="./assets/images/bg-pattern-bottom.svg"
          role="presentation"
          alt=""
          class="c2"
        />
  

  and then remove background properties from body. this way, background circles won't go bananas on smaller screens. keep in mind that's my approach and there are definitely better approaches you may come up with.

  • use width:100% and then max-width: ... px instead of giving a static width to your card, so card's layout won't break on small devices, right now your card leaves side scroll on small screens.

  • I don't recommend using static heights for elements unless it's really crucial.

  • for stats, I think you'll be better off with something like

   <ul class="card-stats">
          <li><span>80k</span> <span>Followers</span></li>
          <li><span>803k</span> <span>Likes</span></li>
          <li><span>1.4k</span> <span>Photos</span></li>
   </ul>
  

  instead of using so many divs.

  • give some paddings to body so content won't occupy whole screen on mobile devices.

  ✅ Also I opened a pull request to your repository which will fix these issues

  I hope this was helpful

  Marked as helpful

  2

  ---

  

  lmac-1•10

  @lmac-1Posted almost 3 years ago

  @Yazdun Hi Yazdun, thank you so much for this feedback, it was SO useful and I learnt a lot! I'm so sorry but I've only had time to finish going through it all properly today.

  I haven't accepted your pull request because I wanted to combine your advice with Mike's. I just made some changes this week on GitHub and hopefully, you can see an improvement :) open to any further feedback!

  Thanks again!

  https://github.com/lmac-1/profile-card-component

  0
• 

  Mike Chen•20

  @mhchenPosted about 3 years ago

  Looks really solid!

  HTML

  Everything looks pretty good here, have two suggestions:

  1. Make the person's name an <h1>. It's the most important element on the page
  2. Rarely used but the labels/stats could be dl/dt/dd – would be <dl> for the enclosing element, <dt> for the label (followers, likes photos), <dd> for the numbers

  CSS

  Well organized overall!

  One improvement I'd recommend: there are a few magic numbers in here. Namely, the 87px for .card-stats and the 53px in .card-header img. The latter one is easier to address.

  Basically the 53px only works because the image is a certain size (106px). If we were ever to swap out a different sized image here, the CSS would break.

  If you replace the left and bottom properties with:

  bottom: 0;
  left: 50%;
  transform: translate(-50%, 50%);
  

  You'll get the same centered effect with no magic numbers. I realize you might not have gotten to practice much with translate but this is a really powerful technique for centering.

  The other issue is harder to solve: it's because .card-content doesn't stretch to fill the whole length of the card, and it probably should. The best way to solve this is with a flexbox column approach on the parent element. Happy to go into this more if you're interested!

  Background position

  Hard to imagine a better solution than the one you came up with! Glad you learned about multiple bg images

  Marked as helpful

  0