Social proof section using html and sass :)

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 4 years ago

  Hi Thomas,

  One project every day is certainly ambitious, which I admire. But just be careful you don't burn out!!

  This looks pretty good, but there are some points for improvement to take away:

  • All your text is in divs at the moment, which is not a valid html tag for text. Use paragraphs, headings or list items
  • I would wrap all content in one wrapper and center it on the page using flexbox. Then everything would line up on each side and would stay vartically centered. Better than using top margins for layout.
  • The stars need the alt attributes emptying. At the moment, screenreaders would announce every one like "image, stars - image, stars - image, stars, image..." which makes for a poor user experience. Change it to alt="" instead and assistive tech will know to skip over them. It's fine in this case as the 5 stars is already in the written description.
  • Similarly, in the people avatars, you don't need to say the word 'picture' in alt text as it's already being announced as an image.

  I hope that's helpful. If it is please upvote, I'm working on my mentoring :)

  1

  ---

  

  Thomas Boittin•265

  @tboittinPosted almost 4 years ago

  @grace-snow Thanks a lot Grace ! You just gave me insights I would take a long time to get!

  Will make these changes right now.

  About the 1 project per day challenge, I saw some people make it so I guess it's feasible. I'm planning to do easy stuff like this which doesn't take much time to save time for harder projects. I'll make them step by step spread on several days.

  It's nice for more experienced dev to give their opinion, thanks a lot!

  0

  

  Thomas Boittin•265

  @tboittinPosted almost 4 years ago

  @grace-snow Following your advice i made these changes:

  • added p tags where text was directly inside divs

  • put ul/li tags for multiple elements (reviews and ratings)

  • add a reset.scss file to make sure the changes don't alter the layout

  • created a wrapper and used the align-items/justify-content combo to center the page. In fact, the layout moved down a bit to look more like the design, which is nice !

  • removed the useless alt content.

  Thanks again for your help !

  1

  

  T

  Grace•31,270

  @grace-snowPosted almost 4 years ago

  @tboittin good job, well done!

  Remember to keep upvoting everyone's feedback you fnd helpful like me and @kyrylolvov on this so we build up our mentoring 😉

  1

  

  Thomas Boittin•265

  @tboittinPosted almost 4 years ago

  @grace-snow Sure I do

  0
• 

  Kyrylo Lvov•160

  @kyrylolvovPosted almost 4 years ago

  Hello there! 😀

  You did a great job on this project! 🏆

  What I might suggest is looking at 1040px resolution, since there is a little issue with stars that behave differently frome each other. 🙂

  1

  ---

  

  Thomas Boittin•265

  @tboittinPosted almost 4 years ago

  @kyrylolvov thanks for the review ! I will look for a solution to make it better :)

  1

  

  Thomas Boittin•265

  @tboittinPosted almost 4 years ago

  @kyrylolvov I added a min-width to the stars' div so it won't behave weirdly anymore.

  Thanks for helping me improve my solution !

  1

  

  Kyrylo Lvov•160

  @kyrylolvovPosted almost 4 years ago

  @tboittin You are always welcome 😊

  1