Hello
Really good job overall
Here are some suggestions:
- Insert a css reset at the start of your stylesheet
- Never ever have font sizes in pixels. Use rem (or sometimes em)
- The features section element should really live within the main element
- It's better to have an sr-only h2 in that section and use aria-labelledby on the section instead of aria-label. This is partly because aria-label isn't reliably translated into different languages, and partly because that would improve the page structure. At the moment, the parent heading of those feature h3s is actually the "mechanical wireless keyboard" h2. That might be fine, but I think having a "Features" heading would be better.
- There is horizontal overflow happening on some screen sizes. Overflow-x hidden on the body would probably fix that
- I don't think you're using the right elements for the buttons. Those should almost certainly be anchor tags as they would take people to another page (trigger navigation).
- Most important change of all
<img src="./assets/shared/logo.svg" alt="logo" class="header__logo">. This means nothing to search engines and assistive tech users. The brand logo (and therefore name) is the most important information on the page. Fix that alt text ;)
Good luck
Marked as helpful
@gchristofferson
Posted
Hey @grace-snow!
Thank you for that awesome feedback!!!👍😀
Some of your suggestions I implemented were changing the font-sizes to rem, moving the features section to the main element, changing the aria-labels to aria-labelledby, adding a h2 to the features section with a sr-only class, adding the overflow-x hidden to the body and fixing the alt text for the logo and using anchor tags instead of buttons for the links. Your suggestions helped me improve the structure and accessibility of my solution. Thank you so much!! 🙌
I had a couple of questions about your feedback if you have time to answer:
- When changing my font sizes to rems, I also changed the line-height from pixels to rems. Was that a good idea, or would ems be better in this case so that the line-height scales in proportion to the text??
- I used your sr-only class from your solution to the 3 card column preview. I hope that was ok😳. I understood what the class did except for these two properties:
--
clip: rect(1px, 1px, 1px, 1px)andwhite-space: nowrap. When I toggled these off, I saw no difference. I was just wondering what these do actually? - About using anchor tags for buttons, I am a little confused because I thought using a button was better for buttons, since they have accessibility built in. Should I only be using buttons for user actions and not links?
- I use a hard reset in my style sheet:
*, *::before, *::after { box-sizing: border-box; margin: 0; padding: 0; }Is this not recommended? Resets are another thing I'm confused about. Some recommend the above, others say it should be avoided, while others use very long reset scripts. What is best practice here???
Well done @gchristofferson , fast work!
- Line height should be unitless or % (eg line height 1.5)
- That’s not my snippet, it’s widely established across the web. Those properties address browser quirks to ensure content is visually hidden
- Al interactive elements “have accessibility baked in”. But you have to use the right ones for the job. Always anchor tags for navigation, buttons for actions
- It’s all opinion, but I think it’s better to have a larger reset to remove browser default styles. I like Andy Bells modern reset , but adjust it very slightly for myself
Marked as helpful
@gchristofferson
Posted
Thanks @grace-snow! You're the best👍 I'll be fixing those line-heights too. 😊
