social-links-profile-main

Community feedback

• 

  Kirsten ✨•460

  @ofthewildfirePosted 9 months ago

  Hi there. Your solution looks great, I just have a few small suggestions.

  When you zoom in (as some users do on sites), your component is chopped off a bit at the top, this is because there is no space for it to "go" - to fix this add a min-height: 100vh as opposed to a height - those two confused me to heck and back, but they are different :)

  An updated solution would be something like this:

  body {
      max-width: 90rem;
      display: flex;
      justify-content: center;
      align-items: center;
      min-height: 100vh;
      background: var(--Off-Black);
  }
  
  

  Your .container element. First off, using a div is likely not the best option. Every page needs a main element, it is the entry point, and since this is just one component, its the place to use main -> You also are using a fixed width for your card, as a general rule fixed widths are not the best / actually most times you should avoid it. Instead, it would be better to use a max-width with a rem value. eg. max-width: 25rem to have the save consistency. Height value set to also is also not needed and does not add anything.

  The use of position is also so finicky and in my experience just makes it hard as all heck to make things response, and again its not neccessary, HTML will work with you to get all the proportions right, adding extras in this regard is just more complexity than is needed!

  An updated rule set for the container element would be something like:

  .container {
      max-width: 25rem;
      background: var(--Dark-Grey);
      border-radius: 20px;
      text-align: center;
  }
  
  

  Adding a width to your image is not needed at all. The margin bottom is also not needed at all.

  Within your solution you use a lot of px values for your font sizes, this is not the best idea, you can read more here -> Why fonts should never be in pixels - so changing these to the recommended rem would be best!

  We also don't need the padding top and bottom on the .container p selector, I think you did it to add space, you can use something like Grid and gap for that!

  You'd add the Grid to the .container element, so, something like this: ->

   .container {
      max-width: 25rem;
      padding: 2em;
      background: var(--Dark-Grey);
      border-radius: 20px;
      text-align: center;
      display: grid;
      gap: 1.25rem;
  }
  
  

  You can likewise do the same within your ul element to add space between your li elements.

  Overall, it was a good solution. Hope some of this was helpful!

  Happy coding!

  Marked as helpful

  1