Responsive design huddle Landing Page with pure CSS

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 2 years ago

  Hi

  I know you asked about css but I see issues with html first that need correcting

  1. Use landmarks. This at least needs a main element but really the logo should be in a header and everything else in a main.
  2. Please Indent your code consistently 🙏 It is very difficult to read at the moment. Your code editor can even do this formatting automatically for you
  3. You don't need the background class to apply a background on this. You can just apply that bg to the body
  4. Alt text should never include words like "image" because they are already an element with an image role. The logo needs alt that says Huddle because that's the writing on the image
  5. As you've wrapped the logo in an anchor (href on that should be / btw not #) you need to label it with the link destination. You can either do this in the logo alt eg "Huddle - Home" or use an alternative labelling technique such as this text on an aria-label on the anchor tag itself.
  6. You don't need to wrap the button in a div. Not a problem, just unnecessary when you could make btn display block or inline block
  7. The social links need accessibly labelling. At the moment it is like they have no content. Usually this is done with some visually hidden text inside the link and aria-labelledby on the link; or aria label on the link
  8. You are importing a HUGE library of icons just to be able to use 3 of them. It would be better to download the 3 svgs you need instead
  9. It is more performant to link fonts in the html head than import in css
  10. The title should be up at the top of the head

  1

  ---

  

  Arash Kazerouni•140

  @arashKazerouniPosted almost 2 years ago

  @grace-snow thanks to Grace for this complete guide.🙏 just to complete the point,

  • focus more on semantic HTML,
  • choose more relatable names for elements,
  • keep things as simple as you can,
  • last but not least, when you're coding, please consider that somebody else would like to read your code in the future.

  0
• 

  T

  Grace•31,270

  @grace-snowPosted almost 2 years ago

  In the CSS you're just really over-complicating it I think.

  This only needs ONE media query. You should be working mobile-first. That means the mobile styles should be the default ones. Then add a min-width media query when there is room for the layout to change as you enlarge the viewport. Media queries should be in rem/em, never px.

  Overall, try to focus on when to use padding vs margin; and focus on consistency. Think about what is reusable. For example, it is very common to have one class that sets a max-width, little side padding and margin-inline auto. That class can then be reused on wrapper elements to limit the content width throughout the site.

  I went through and made these changes in browser to improve it, but the media query is definitely set at the wrong place here. There should be one around... 50rem, something like that.

  
  /* style.css | https://itsnooshin.github.io/huddle-landing-page/style.css */
  
  @media (max-width: 1214px) {
    .img-logo {
      /* padding: 2.6rem 1.5rem 4rem 1.5rem; */
    }
    .conatiner-main {
      /* padding: 10px 20px; */
      /* max-width: 100%; */
      /* width: 100%; */
    }
    .container-title {
      /* width: 900px; */
      /* line-height: 36px; */
    }
    .conatiner-lead {
      /* width: 700px; */
      /* margin: 20px auto; */
    }
    .btn {
      /* width: 60%; */
    }
    .btn {
      /* width: 100%; */
    }
    .social-media {
      /* align-items: center; */
      /* width: 100%; */
      /* padding-right: 0; */
      /* margin-top: 1rem; */
      /* margin-bottom: 2.5rem; */
    }
  }
  
  .img-logo {
    /* padding-left: 67px; */
    /* padding-top: 4rem; */
    padding: 2rem var(--content-side-padding, 2rem);
  }
  
  .conatiner-main {
    /* gap: 60px; */
    /* padding-left: 67px; */
    gap: 3rem;
    padding: 0 var(--content-side-padding, 2rem);
  }
  
  @media (max-width: 768px) {
    .text-container {
      /* max-width: 1100px; */
    }
    .container-title {
      /* max-width: 500px; */
      /* padding-left: 4rem; */
      /* padding-right: 4rem; */
      /* margin: 0 auto; */
      /* text-align: center; */
      /* padding-bottom: 1rem; */
      /* line-height: 1.5; */
    }
    .conatiner-lead {
      /* width: 370px; */
      /* margin: 0 auto; */
      /* text-align: center; */
      /* line-height: 1.7; */
      /* font-size: 1.3rem; */
    }
    .btn {
      /* width: 40%; */
      /* margin: 20px 0; */
      /* margin-bottom: 4rem; */
      margin: 1.5rem 0 4rem;
    }
  }
  
  .text-container {
    /* margin-top: 3rem; */
    /* justify-content: flex-start; */
    justify-content: center;
    gap: 1.5em;
  }
  
  .container-title {
    /* width: 487px; */
    /* line-height: 60px; */
    line-height: 1.2;
    max-width: 20ch;
  }
  
  body {
    /* line-height: 1; */
    line-height: 1.7;
  }
  
  .conatiner-lead {
    /* width: 520px; */
    /* margin: 1.5rem 0; */
    /* line-height: 27px; */
    max-width: 50ch;
  }
  
  .btn {
    /* padding: 1.3rem 4rem 1.3rem 4rem; */
    padding: 0.5em 3em;
  }
  
  .social-media {
    /* padding-right: 5rem; */
    /* padding-bottom: 2.5rem; */
    padding: 3rem var(--content-side-padding, 2rem);
  }
  
  .container {
    max-width: 70rem;
    margin: auto;
  }
  

  0
• 

  Hassia Issah•50,650

  @HassiaiPosted almost 2 years ago

  Add aria-label attribute aria-label=" " to the <a> of the social media icons to fix the error and accessibility issue. The value of the aria-label is the description of the image. For more aria-label click here

  Replace <div class="background"> with the main tag to fix the accessibility issues. click here for more on web-accessibility and semantic html

  Reduce the screen size for the media query, and give the body a background-size of contain.

  give the background-image you gave to .background to the body.

  For the left-side and right side spaces of the page give . background a max-width of 1440px, a width of 80% and margin: 0 auto;

  .background{
  max-width: 1440px;
  width: 80%;
  margin: 0 auto;
  }
  

  Hope am helpful.

  Well done for completing this challenge. HAPPY CODING

  0

  ---

  

  nooshin bakhtiari•110

  @itsnooshinPosted almost 2 years ago

  @Hassiai Thank you for your help :*

  0