👋 Hello!
Congratulations on completing the challenge. 🥳
There are a few things that I think would make this even better:
- Use only one <h1> per page, for accessibility and SEO. In this case I'd see "Sedans", "SUVs" and "Luxury" of the same logical level, so I'd approach this by adding a <h1> with a screen reader only class (visually hidden but still picked up by screen readers), and have "Sedans", "SUVs" and "Luxury" as <h2>. Here's a good video by Kevin Powell sharing more about headings.
- In the footer, to be more semantically correct, use <p> instead of <div> for texts.
- For the rounded borders, an alternative approach is to set
border-radius: 8pxandoverflow: hiddenon the parent. This could make things a little easier as the border-radius would not need to be updated in the media query. - For the anchor elements CSS, because the font-size isn't explicitly set and there's a fixed pixel height and width set, there will be text overflowing out of the element when the base font-size increases (i.e. someone who increased font size on their device). Most cases of fixed widths or heights could be replaced with
min-widthormax-widthetc, so that it scales better. Another approach would be to usepaddinginstead. I'd also recommend usingemorremunits instead ofpxfor better accessibility.
Nice work and keep up the good job! I hope this helps and happy coding! 😊
Marked as helpful
0
Magdalena• 500
@magsaram
Posted
@Milleus Hi Dave! Thank you for opinion :)
You're right, there should be one h1, I know it, I use it three times because there was no one one main title, so I was stupid :D But I'll remember now, h2 is better on this kind of projects :)
And I know, there must be <p> in <div>... I don;t know, why I didn't use it, must be more precise.
Thanks for hint about overflow-hidden! I know this property, but I forgot it now :/
Anchor - yep, I understand, I'll try to remember <3
0
