Design comparison
Solution retrospective
I normally rely on the Figma design file to nail the spacing between elements, but this time without it provided I had to use my best judgment. I believe I did well with the spacing between elements and it looks fairly close to the design.
I also learned about the picture and source element from the web.dev resource, and I will continue using it for other projects.
At the custom breakpoint I had set, I noticed that once the layout shifted, that there was a strange gap under the image for the first couple of pixels past the breakpoint before it grew. I noticed it seemed to be due to the text wrapping at the point of the breakpoint. I decided to use the breakpoint to decrease the amount of padding once the layout shifted and it didn't produce the gap between the image and the bottom of the card.
Community feedback
- @grace-snowPosted about 2 months ago
Hi, this looks really good when I preview in browser, we'll done. Here are some suggestions for you:
- this is a single card component and would never be used to serve the main heading on a page. That means it shouldn't have a h1. Use a lower importance heading level like h2 instead.
- people using a screen reader may struggle to understand the old price or why there are two prices in the card. I recommend you add some visually-hidden (sr-only) text in a span before the old price to say that's what it is.
- the cart icon in the button must be treated as decorative by giving it an empty alt attribute. Because you have added alt there it is changing the accessible name of the button which means voice control users won't be able to activate it.
- make sure keyboard focus on that button is styled with an obvious outline (usually offset by a few px). Changing a background colour is not obvious enough for keyboard users to see where they are on a page.
I have only inspected the code briefly and previewed on mobile so I can’t comment with any certainty on the responsiveness or reflow of this. But it looks like you may be switching to 2 columns too early to me from the classes.
Marked as helpful1@JYLNPosted about 2 months ago@grace-snow Thank you for the feedback! Your insight and knowledge has been super helpful on my past few solutions. I've always struggled with accessibility in my coding so having this professional feedback has been amazing and has given me a lot to work with and continue learning how to make it better. I will make changes based on your feedback and continue to work on implementing it on further solutions. Thank you again!
0@grace-snowPosted about 2 months ago@JYLN I've noticed something else. Because you've set the component's max-width in pixels instead of rem, the display breaks really badly when someone changes their browser or device default font size. Try it out and you'll see (browser settings > font-size > set it to 200% or 32px).
That's also why we always recommend setting media queries in rem or em not px.
I am also seeing some really strange performance lags on the images when I resize the browser and am not sure what's causing it. I'd be tempted to add aspect-ratios, object-fit (and maybe a height:100% on larger screens only) to the img to see if that improves. Also it's entirely possible that is being caused by my poor connection and overloaded computer coz I have sooooo much open right now!!
2@JYLNPosted about 2 months ago@grace-snow I was also thinking the px issue when I woke up this morning and was reviewing my code. I understand what you mean about the image lag, I was seeing some strange things as I was building it yesterday but I'm not sure if it was lag either. I will do some playing around and see if I can find some optimizations. Thank you!
0
Please log in to post a comment
Log in with GitHub
Join our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord