Design comparison
SolutionDesign
Community feedback
- @grace-snowPosted over 3 years ago
Hello
Here's my suggestions, I hope they're helpful
- change .page div to be a
<main>
tag instead. That should fix the accessibility issues - Re-write or remove the alt text. You need to have the attribute on there, but it's OK to be left intentionally blank on unimportant images like these. If you do want to include descriptions of the images, they need to be human-readable, not hyphenated words like that
- change the buttons for anchor tags as these would take users to another page. Anchor tags are for navigation, not buttons
- Your solution doesn't work on my mobile because of the min-width 375px. There's no need to ever have a min-width on the body.
- font size must never be in px. Use rem (or sometimes em when you want the size to inherit from it's parent) every time.
- give buttons a border at all times, not just on hover, that way all the content won't move
- remember to add obvious focus-visible styles whenever you have interactive elements
- It's better to use flex-basis (and other flex properties) instead of width on flex-children. It won't make much difference in this particular challenge, but worth following that practice so you know for future
- I recommend using rem for vertical margin and padding as well instead of percentages so you have better control. _(Really it's good to use rem for everything where you might ordinarily use pixels so content always scales when users change their text zoom settings)
- A simpler way to do the button colors on this would be to use mix-blend-mode screen. That creates a cut-out text effect, so you wouldn't have to define a different color on each button.
Marked as helpful1 - change .page div to be a
Please log in to post a comment
Log in with GitHubJoin 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