Design comparison
Solution retrospective
Would love to receive valuable feedback on a recent project:
- Mobile-first,
- Added light ':hover' effect on each card and rating.
Community feedback
- @grace-snowPosted almost 4 years ago
Hi,
Your solution looks pretty nice on mobile, but quite narrow on my small screen. I'd recommend some padding or margin on an wrapper and letting the content be full width (maybe with a max width). Also, adjust line height on the heading.
It looks from the desktop preview you've got some spacing and alignment issues? Maybe revisit that to see if you can get closer to the design (remember you can zoom in/out on your own screen to test it out)
Your html is really well structured and uses semantic tags nicely.
CSS is in a really strange order though.... Its more usual to put all the base styles at the top and then add media queries under, or to use media queries throughout on each element that needs it. And I wouldn't expect to see any use of
!important
which will wreck your specificity and cause big problems in larger projects. If you find you're having to use important, it's a sign there is something wrong with the css and specificity is high somewhere else.When you say you've added a hover effect, I strongly advise against that for this callenge unless you're adding in some kind of interactive behaviour. The whole purpose of hover and focus effects are to indicate interactivity and there is nothing interactive on this challenge. That means you're creating an anti-pattern by adding the hover - potentially damaging UX rather than enhancing it as people would be trying to click stuff and presuming it's broken for them.
I hope all this is helpful advice. If you go back and make a few tweaks, preferably with a css tidy up, this will be a good portfolio piece
3@egxperiencePosted almost 4 years ago@grace-snow Thanks a lot for your feedback. I'm going through CSS now to see what is the issue here related to the usage of !important as without it 'margin' not working for "rating" section. Will also rearrange spacing in wider screen, maybe by adding some values to 'container' settings and ask again to check if you mind.
0
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