Hi Kathryn
Here are some tips that should help you 😊
- make the component main and the attribution footer. There is no need for a separate main div although it’s doing no harm
- give the product image a better alt description. That’s an important bit of content so needs a proper description. Alt should be a human readable description that would allow someone to picture the image even if they can’t see it
- the cart icon on the other hand is decorative so alt should be left blank on that.
- remove the h1 from the price. Use paragraph there. You should only have one h1 per page. And only use headings when they are actually a title for some other content. Headings are the most important feature in html for giving content structure. Think about them like a contents page on a long document - each heading has to be in order to communicate the structure of content
- the strike through text should be wrapped in a
del
element. Screenreader users are not notified about text that has a line through, so this also needs a span with an sr-only class on it and writing to inform users that’s the old price (otherwise they would just hear two prices read out which would be confusing) - you don’t need role of main on the main element. It already has that role built in.
- remove height from the button. Instead use padding in a unit like em (or rem). This will ensure the button stays responsive even if a user changes the text sizes
- never ever have font size in px. Use rem. Overall I think you are using px too much and would be better using rem more in other places too
- padding is for internal spacings (like inside a box) and margin is for external spacing (like creating space between paragraphs). Often (not always) margin will only go in one direction but padding will be on all sides.
- If you ever find yourself having to use
!important
it is highly likely there is something wrong elsewhere in your css. That indicates too high specificity elsewhere, so go fix it there. Try to avoid important at all costs - usually a css reset at the start of a stylesheet sets img tags to display block and max-width or width 100%
- consider using the object fit property on the main product img, along with height 100%
- line height is usually unitless, like 1.5, or occasionally in %, not in rem. not wrong, just unusual. It must never ever be in px though
- letter spacing is usually in em so it scales with the current font size
- I wouldn’t expect any max-width media query on this. If you are working mobile first, mobile styles are the base and there is no need for a media query to target that size
- there is no need for a tablet and a desktop media query on this. All that needs to change on larger screens (whenever there is room for the side by side layout) is: max width of the component, flex direction, width/flex properties of the two halves
Marked as helpful