Community feedback

• 

  Andrés Gutiérrez Ramírez•2,470

  @AGutierrezRPosted over 1 year ago

  Hello there 👋. Good job on completing the challenge!

  I have some suggestions about your code that might interest you.

  HTML:

  • It should be one <h1> element per page. You could use a <ul> to display the stats.

    <ul class="profile-card__stats">
      <li class="profile-card__stats-item">80K <span>Followers</span></li>
      <li class="profile-card__stats-item">803K <span>Likes</span></li>
      <li class="profile-card__stats-item">1.4K <span>Photos</span></li>
    </ul>
    

    Something like this could be.

  • You could replace the <article id="article-center"> for a <div> is more semantic in this situation.

  CSS:

  • Try not to limit the width of the body. This could help you with the background positioning

  • You could use multiples background-image for the page background, for example:

    body {
      position: relative;
      background-color: var(--clr-dark-cyan); /* use you color variable here */
      background-image: url(../images/bg-pattern-top.svg),
        url(../images/bg-pattern-bottom.svg);
      background-repeat: no-repeat;
      background-position: right 40vw top -37.5rem, left 42vw top 14.375rem;
    }
    
    @media only screen and (min-width: 90em) {
      body {
        background-position: right 52vw top -32.25rem, left 48vw top 23.125rem;
      }
    }
    

    This could help you to understand multiple patterns in the background. You also could use CSS Grid and a absolute element to make the background!

  • You can use the same trick for the card top pattern.

  0

  ---

  

  Jean Fischer Dirimasi•70

  @jfcode101Posted over 1 year ago

  @AGutierrezR thank you for your feedback!

  Table of content

  • HTML
  • CSS
  • Appreciation

  HTML

  HTML

  • About one <h1> element per page, I totally agree with you, and I'm about to change that.
  • Changing <div> to <article> , the accessibility report and HTML validation report were not in agreement when I did that before. Since <div> is a non-semantic element. This article HTML Semantic Elements, jump to <article> section, it'll help you.

  CSS

  CSS

  • Try not to limit the width of the body. This could help you with the background positioning: The project style-guide.md itself specifically requires or suggests a width of 1440px, and given the fact that this is just a component I see no harm in doing so.
  • Suggesting to use multiple background-image, does that have anything to do with this project?

  Appreciation

  Appreciation

  Thank you very much for feedback!
  I appreciate the time you took to review my code! I like the code snippets you provided. However, Don't you think there's more than only one right way to do this!?

  0