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
@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
