Designcard

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  👋 Hi Simon!

  🎉 Congratulations on completing this challenge! I have some feedback to help you improve this solution:

  • Accessibility
    • 👍 Good job on taking care of the decorative image.
    • I would recommend wrapping everything in main landmark since everything is the main content, except your attribution.
    • This should not be a section since there's no heading. You can swap the section with ul and each p with li.

  <section>
    <p><span>10k+</span> companies</p>
    <p><span>314</span> templates</p>
    <p><span>12m+</span> queries</p>
  </section>
  

  • CSS
    • This isn't the correct way to use both font families. You should choose either Inter or Outfit as your main font family (body's font family). Then apply another font family on the correct element (based on the design).

  body {
    font-size: 15px;
    /* 👇 This font family don't get applied */
    font-family: 'Inter', sans-serif;
    /* 👇 This is the only font family that get applied */
    font-family: 'Outfit', sans-serif;
    ...
    /* One tip, you can remove the below property */
    /* body element by default always fill the entire page */
    width: auto;
  }
  

  • To understand the above topic in more detail, I would recommend learning about the first letter of CSS (Cascading) (cascading principle).
  • Don't limit the height of the body element, it will not allow the users to scroll the page if the page content needs more height. Use min-height instead. To see the issue, see your solution on mobile landscape mode (view).
  • Use rem or sometimes em unit instead of px. Using px will not allow the users to control the size of the page based on their needs.
  • I would recommend referencing each element that you want to style using class instead of > operator.
  • I am guessing that you are using the width of the design (375px) as your breakpoint for @media query. Keep in mind, that those sizes on the style-guide.md have nothing to do with the media query. They are just telling you what sizes those images were created. Your job is to make sure that our website is responsive on all screen sizes.
  • Best Practice
    • I would recommend deleting all your commented code or uncomment and using it instead. The general rule of thumb is if you don't use it, delete it.
    • Write your stylesheet or styling for mobile layout first and then use min-width media query to style bigger screen size. This method is called mobile first, which is popular nowadays and often makes you write less code.

  That's it! Hopefully, this is helpful!

  Marked as helpful

  1

  ---

  

  Simon•50

  @simon-baumhauerPosted almost 3 years ago

  @vanzasetia That was more than helpful, thank you so much for this thorough answer. I appreciated it.

  0

  

  Simon•50

  @simon-baumhauerPosted almost 3 years ago

  @vanzasetia "Use rem or sometimes em unit instead of px. Using px will not allow the users to control the size of the page based on their needs." So should I use in all cases rem or em?

  "I would recommend referencing each element that you want to style using class instead of > operator." Why should I not use the operator?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @simon-baumhauer Yes, but not in all cases. What I'm trying to say to you is to avoid using px unit because of the reason that I already mentioned and rem or em would be a better option.

  Yes, you can use the > operator. What I'm trying to say to you is to keep the specificity of your stylesheet as low as possible, by referencing any element that you want to style by using class. In this case, it is possible to reference any element by directly writing the class name.

  Marked as helpful

  0
• 

  Andjela•90

  @AndjelaAxyPosted almost 3 years ago

  You should set the width 50% for your div with class image.

  Marked as helpful

  0
• 

  Michael Bard•0

  @mikebardpythonPosted almost 3 years ago

  @vanzasetia thank you, if we input alt="ladies enjoying their work" would the VoiceOver screen-reader still pronouce the image with null alternative text or would it read out what "ladies enjoying their work"?

  0

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @mikebardpython If there is text content inside the alt attribute, all screen readers will pronounce the image, not just VoiceOver.

  The below image will be invisible for all screen readers
  <img src="/image/tree.png" alt=""  aria-hidden="true">
  
  The below image will be visible for all screen readers
  <img src="/image/tree.png" alt="A tree that is being cut down by a man using a sawing machine">
  

  Actually, the real concept that you should understand is that when the image is a decorative image and how to handle it, and when the image is informative image, and how to handle it. To learn more you can check this tutorial from WAI about image.

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @mikebardpython I would recommend replying my comment instead of creating new feedback by clicking the REPLY text.

  0

  

  Michael Bard•0

  @mikebardpythonPosted almost 3 years ago

  @vanzasetia thank you much appreciate for the feedback

  0
• 

  Michael Bard•0

  @mikebardpythonPosted almost 3 years ago

  In the html for the image in dont understand why you are setting aria-hidden="true"? referencing the below discussion on stackoverflow seems like it would be okay to do this for icons but we would want screen readers to tell the users about this.

  Im relatively knew so any knowledge you could share would be appreciated.

  https://stackoverflow.com/questions/31107040/whats-the-difference-between-html-hidden-and-aria-hidden-attributes

  0

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @mikebardpython aria-hidden="true" in this case is to prevent the VoiceOver screen-reader (Apple's screen-reader) to pronounce the image with null alternative text. Otherwise empty or null alternative text would be enough.

  0