Hey, this looks really good, well done!
Only small suggestions quickly
@media screen and (min-width: 535px) {
.wrapper {
/* grid-template-areas: "cyan red" "cyan red" "orange blue" "orange blue"; */
grid-template-areas: "cyan red" "orange blue";
}
}
I changed this above, because you don't need 4 rows at this screen size. The items were actually in the incorrect grid areas.
Figure is incorrect semantics for these cards. Only use figure when you need figcaption. They should just be Divs IMO, there's not enough content there to justify articles
The main__figure-img images are decorative / meaningless so should not have alt text. Instead, change to alt="" and aria-hidden="true" or role="presentation"
Good luck
Marked as helpful
One more suggestion - rather than adding min-height to the grid items like .main__figure at different breakpoints, why not just define that on the grid template? I doubt it would need to change at different breakpoints.
Just an idea
@brendanmadden
Posted
@grace-snow Grace, thank you. That means a lot, I really appreciate it! Truthfully the majority of my improvement over the last couple of weeks and projects is in very large part thanks to the feedback that you last gave me, so thank you again!
In hindsight it seems silly that I added those extra rows on that grid template 😅 Makes sense - thank you for pointing that out.
I'll make a note about alt text and other attributes on decorative/meaningless images moving forward, and also make sure to only use figure's when I need figcaptions. Hearing the way you think through things definitely helps.
I'm going to have to play around a little bit with your last suggestion there because I can't exactly picture it yet, but I'll dig into it! Setting height and width values has been one of the more challenging things for me wrap my head around so this will be great practice.
Thanks again Grace!
