Responsive layout using CSS Grid & Flexbox & little bit of JS

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted over 2 years ago

  Hi

  This looks good on my mobile but there are several important accessibility lessons you need to learn

  • a logo should not have an empty alt. It’s possibly the most important content on the whole page - tell me what this site/product is called
  • you must never ever attach click event listeners to non interactive elements. With a nav toggle, it needs to be a button inside the nav element. That button needs to have an aria-expanded attribute that toggles on click and preferably an aria-controls attribute pointing to the ID of the list it is toggling. It also needs accessibily labelling (eg with sr-only text inside the button like “toggle navigation”)
  • there is rarely if ever a need to repeat html. You should be able to use the same nav html for mobile and desktop. And it should not be in an aside (I’m not even sure it’s valid html to have an aside landmark inside a header landmark tbh)
  • I would consider the testimonial people images valuable content. They deserve their name in the alt. The 4 image grid at the bottom looks decorative though so I’d leave those alts blank
  • consider how you could make the testimonial markup more meaningful. Perhaps a blockquote? You could put this in a figure and wrap the persons name and image in a figcaption (although figcaption semantics aren’t that widely supported). Alternatively you could put the persons name above their blockquote in the html but reorder it to visibly sit underneath with flexbox order. Just ideas
  • it’s essential that those social links are accessibly labelled. Again sr-only text would be good here
  • as social links usually take you away from the site you’re on it’s worth adding target _blank to open them in a new tab. When you do that make sure you also add rel=“noopener” for security reasons
  • as you only need a few tiny icons, it’s probably worth downloading them as svgs for this challenge rather than importing a huge icon library (if they are not included in the starter files)
  • speaking of starter files, it’s best to use these as the base for your project. I can see you’ve removed the gitignore file which is important to keep in most repos. That’s how DS Store has ended up in there (not a problem, just unnecessary) but it will become a huge problem later on bigger projects If you forget to ignore node_modules or a similar directory that doesn’t belong in git.

  I hope these tips are helpful

  Marked as helpful

  2

  ---

  

  P

  Katrien Schuermans•1,210

  @katrien-sPosted over 2 years ago

  @grace-snow Thank you Grace for the thorough look you had at my solution. Your answer has given me great insight in the mistakes I made and surely what I in regards to accessibility left out. I will take your remarks to improve this solution, and keep them with me for future projects. It also showed me that the things I learn from tutorials are also not always good practice.

  0
• 

  Victor•1,035

  @CodeVeePosted over 2 years ago

  @graficdoctor. Great job on this project. I have a few comments:

  • There is a need for more spacing between sections
  • Also the grid was switched around. I don't know if that was a creative choice. If it was an issue then I could give a few suggestions on that.
  • The hover is working on chrome
  • There were also alignment issues.
  • The struggle of doing this with a full-time job is highly applauded.
  • I'd recommend using the Figma design and sectioning the project for your to-do list.

  0

  ---

  

  P

  Katrien Schuermans•1,210

  @katrien-sPosted over 2 years ago

  @CodeVee Hi Victor, thank you for the comments. But they off course leave me with some questions. Would you mind elaborate on them? What do you mean by 'grid was switched around'? Which are the alignment issues?

  0

  

  Victor•1,035

  @CodeVeePosted over 2 years ago

  @graficdoctor . Hi Katrien, you swapped the position of the egg and cup pictures. The Stand out to the right audience header seems to lack padding.

  0

  

  P

  Katrien Schuermans•1,210

  @katrien-sPosted over 2 years ago

  @CodeVee Ha, I totally didn't notice I had swapped the position of the images. I will solve that. The padding was actually another 'issue' I had. Which I didn't realise I could solve by using a padding...

  Thank you for the feedback. Very much appreciated.

  0