Community feedback

• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  Greetings, Roc Tanweer! 👋

  It's good to see you complete another challenge! 😀 Great job on this one! 👍 Overall, your solution looks good and is responsive and it's wonderful that you learned lots of new things (I always do when completing challenges, as well)! 😊 And I like the animation of the mobile menu icon and the custom attribution that you added to your solution! 😄

  A few things I'd like to suggest are,

  • Adding overflow-x: hidden to the body of the page (or something similar) to prevent a horizontal scroll bar from appearing along the bottom of the page when the image changes in the "Features" section.
  • Adding a max-width to the header of the page in the desktop layout to ensure that it isn't too wide on extra-large screens.
  • Using the <button> element (rather than a <div>) for the hamburger menu icon (since it is button and that will improve the accessibility of your solution).
  • Perhaps taking another look at your class names. If you're trying to use BEM, I believe there should only be one element per classname (in other words, something like block__element instead of block__element__element), so you might want to simplify some of your classnames a little. I don't think classnames have to reflect the actual structure of your HTML unless that makes things confusing, so you might be able to do things like header__link instead of header__nav__links__link.
  • Considering taking a look at this StackOverflow page (as it might help you clear up those few errors on your solution report).
  • I noticed you created the accordion in this challenge with HTML and JS (and it works well, kudos!) but if you're interested, you can create accordions with just HTML by using the <detail> and <summary> elements. It's pretty nice and easy, and you can learn more about it from this helpful MDN page.

  I hope those tips help. 🙂

  Of course—keep coding (and happy coding, too) my friend! 😁

  5

  ---

  

  Roc Tanweer•2,500

  @RocTanweerPosted over 3 years ago

  @ApplePieGiraffe Thank you for your words..! I really admire you..!

  • There is some kind of bug which is the reason for that overflow. When it shows, if you click on the ham menu or submit the form or click on the attribution icon, overflow will be gone(There shouldn't be any overflow because nothing is overflowing). It started happening when I added animation to feature section and hero section...
  • I will add max-width right away.
  • I did header__nav__links__link for SCSS nesting...I saw your words with grace and now I know how to fix that
  • I will look at Stack Over flow [Edited: I looked at their answers, but I couldn't understand. If I add a text in an icon link, then it will be visible. If I hide it visibility:hidden then it will take space. What should I do?]
  • I will try making another accordion with your advice

  Happy coding too..!

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  @RocTanweer

  Alright—that sounds good! 👍

  As for the icons that are wrapped in anchor tags, you could add a <span> inside the anchor tag (next to the icon) with some text that describes the purpose of the link and then visually hide it from users with a sr-only class (you can easily copy-and-paste and sr-only class from the Internet), like this,

  <a href="/">
    <i class="fa fa-icon"></i>
    <span class="sr-only">Description</span>
  </a>
  

  You could also add aria-hidden: true to the icons so that screen readers don't get confused about it or anything. 🙂

  Looking forward to your next solution! Cheers! 🙌

  1

  

  Roc Tanweer•2,500

  @RocTanweerPosted over 3 years ago

  @ApplePieGiraffe Thank You..!!!

  0
• 

  MasterKrab•940

  @MasterKrabPosted over 3 years ago

  Hello Roc Tanweer, great job on this challenge!

  I think I can help you with a couple of suggestions.

  • You can add an aria-label to links with descriptive text for screen reader users.

  <a href="#" aria-label="Description">
    <i class="icon" aria-hidden="true"></i>
  </a>
  

  • I think the "More info" button should be a link because it will probably send you to another page with more information.

  • The download-box should be an article or section and not just a div.

  • The hover effects should only be on desktop because on mobile is problematic and does not make sense to place it.

  I hope you find it useful, let me know what you think.

  Happy coding!

  1

  ---

  

  Roc Tanweer•2,500

  @RocTanweerPosted over 3 years ago

  @MasterKrab I cleared the accessibility issue with social links

  I will learn more about semantic HTML5

  Thanks for your feedBack 😀

  Happy coding too!!

  1