Social Proof Section with CSS Grid, Flexbox, BEM and SCSS

Community feedback

• 

  Vanza Setia•27,755

  @vanzasetiaPosted over 3 years ago

  👋Hi kens_visuals!

  I can't see any hover effects, maybe it's because I'm on mobile device 😅. The only hover effect that I can see is your attribution links.

  I have some feedback on this solution:

  • For any decorative images, you should leave the alt="" and add aria-hidden="true" to make all screen readers ignore those images. In this case, all stars images are decorative images.
  • The Rated 5 Stars... should not be heading. I recommend to use strong tag instead to emphasis the text.
  • For the photos, you can use their name as the alternative text for each photo.
  • To have a better semantic HTML, swap the p tag on content__comment-text with blockquote.
  • The person name and the Verified Buyer should be paragraph elements instead of headings.
  • Never change the root / html font size. It can cause a lot of issues. Here's what accessibility expert (Grace Snow) has said about it.(original comment)

  Yeah, I would never resize html/root font size down to 62.5% mainly because it is completely unnecessary, and it carries risk. You are correct you can mitigate against the accessibility issues by scaling the font size back up with 1.6rem on the body, BUT

  • not all elements inherit font size from the body so you have to remember to manually scale up them as well
  • you need to do extra testing for that reason
  • any rem sizes on third party components or plugins you bring in will need loads more overrides
  • because you’re changing an established default, you cannot guarantee it will work correctly for everyone on every device, operating system, browser and tech set up

  Basically it comes with a lot of risks for almost no benefit.

  There is no reason 1rem needs to equal 10px. As soon as you start thinking entirely in REM - building for alignment, proportionality and spacing - rather than thinking in pixels, the whole point disappears.

  That's it! Hopefully this is helpful!

  Marked as helpful

  0

  ---

  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @vanzasetia 👋🏻

  • [X] For any decorative images, you should leave the alt="" and add aria-hidden="true" to make all screen readers ignore those images. In this case, all stars images are decorative images.
  • [X] The Rated 5 Stars... should not be heading. I recommend to use strong tag instead to emphasize the text.
  • [X] (For the photos, you can use their name as the alternative text for each photo.) Thanks for the advice, yet since I was working on accessibility issues, I decided to add a bit more descriptive text, such as, alt="Profile picture of Irene"
  • [X] To have a better semantic HTML, swap the p tag on content__comment-text with blockquote.
  • [X] The person name and the Verified Buyer should be paragraph elements instead of headings.
  • [X] (Never change the root / html font size. It can cause a lot of issues. Here's what accessibility expert (Grace Snow) has said about it.(original comment)).

  Thanks for the suggestion 👆🏻🙋🏻‍♂️. After you pointed out the issue about changing the root font-size, I started researching to find some workarounds, because it's really convenient to use rems this way, and luckily I ran across this great article that "saved" me 😃 and allows me to use rems as I was used to 👨🏻‍💻

  Thanks for the detailed feedback, it was really helpful. Cheers!

  0
• 

  Dharmik_487•1,740

  @Dharmik48Posted over 3 years ago

  Hey👋,

  Great Job on completing the challenge! It looks really good! Almost perfect. I also really what you did with the hover effects!

  Keep developing🤗👍.

  0

  ---

  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hi @Dharmik48 👋🏻, thanks for the feedback, I really appreciate it 😊✌🏻

  0