Hi, nice looking solution. Here are some improvements I recommend :
- this should all be in a main landmark, not a header landmark. That has a banner role and it's purpose is to hold primary content that repeats across a site. There should be no page-specific content in a header.
- the location isn't appropriate as a heading element, it's just a paragraph. If it was a heading that would make it the title of the content below, which doesn't make sense in this case.
- if making links open in a new tab (unexpected behaviour) you must warn assistive technology users about this beforehand. Usually that's done with some sr-only text in a span inside the link e.g. "(Opens in a new tab)".
- I'd expect to see a modern css reset at the start of the styles. Not strictly needed in this project but don't get into the habit of forgetting it.
- I'm not sure what theme switcher you're referring to in the text above. I can't see that but can see you're linking a js file without any controls on the page so am confused why that's there.
Marked as helpful
2
Levi• 90
@law973
Posted
@grace-snow Thanks for the recommendations! I've changed the solution to incorporate these suggestions and will keep the principles in mind for future projects. I didn't use a proper switch for the theme initially, but there's a button for it now.
0
