Design comparison
Solution retrospective
Hi !
Feedback will be appreciated :) Specially about the grid and media querie
thanx!
Community feedback
- @ApplePieGiraffePosted almost 4 years ago
Greetings, Maria Urdapilleta! 👋
Nice job on this challenge! 👍 Your solution looks pretty good and responds rather well! 👏
Like grace-snow suggested, a little more margin/padding around the grid in the mobile layout of the site would be a nice idea. I think decreasing the space around the grid in the desktop layout (before the site changes to a tablet layout) would be a good idea, too, to leave more room for the testimonial cards. 😉
Keep coding (and happy coding, too)! 😁
1 - @grace-snowPosted almost 4 years ago
Just spotted you asked about media queries!
Its usual to make your mobile layout the base styles and then use min width mq from the point you think the layout needs to change (maybe 600-800px or thereabouts)
You've gone desktop-first and that's fine. But don't then put a min and a max width on your media query. (At the moment, your desktop styles will kick in at 319px wide and below, breaking the layout for small phones). Just use max-width to define the mobile only styles if you've built desktop-first .
I hope I've explained that well
1@mariaUrdaPosted almost 4 years ago@grace-snow Thank you Grace! Really appreciate the feedback.
I noticed about the edges on the mobile version only after I uplodaded the solution. How important is to double check everything haha.
Thanx for the advice on the min and max width. Didn't realize before, but of course makes a lot of sense. I have already updated the code, I hope it looks good now. I've also changed the values on the two versions of mobile devices I had.
0@grace-snowPosted almost 4 years ago@mariaUrda nice one!
Just spotted something else: On mobile some cards have a lot of space at the bottom. Because you've set them to be 1fr high. Why not just let all rows be auto height?
There's some repetition in your css too. Any properties all cards share can be placed in their own
.card
class, like padding and border radius. Then place that class on all cards. That leads to more maintainable css - if the design changes to no radius later, for example, you only have to change one line of css, not 5.Hppe that helps ☺
0@mariaUrdaPosted almost 4 years ago@grace-snow really help!
It actually bothered me the blank space at the bottom of those cards, but never occured to me i could use the auto height to fix them.
About the repetition in the cards, is it possible to use a second class in the same element? Or should be better to use ID for each particular card, and class for those properties shared by all cards?
0@grace-snowPosted almost 4 years ago@mariaUrda best to have multiple classes on one element, never use IDs for styling ☺
1 - @grace-snowPosted almost 4 years ago
Hi Maria,
This looks really nice in the preview but everythings hitting the edge of the viewport on mobile. A little padding/margin on the container should fix that easy enough though.
Good job on the rest, looks good
1
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