Design comparison
Solution retrospective
i am proud of completing the challenge in the shortest possible time
What challenges did you encounter, and how did you overcome them?i faced some challenges in positioning the cards. but i was able to solve them. thanks to the flex feature
What specific areas of your project would you like help with?i woud like someone to show me how to use the gride systeme in he code because i have not mastered it yet
Community feedback
- @grace-snowPosted 6 months ago
It's a real shame you've used flexbox for this layout when it is so clearly a css grid challenge. I recommend you try to refactor this using css grid to stretch the skills it is designed to stretch, using a simpler html structure where the 4 cards are 4 direct children of one grid template.
Other points to note:
- the main heading is one h1 split over 2 lines. Wrap half in a strong or span set to display block.
- try not to skip heading levels. Headings should always go in order to communicate the structure of the content correctly to assistive tech and bots. If the main heading is a h1, the cards should all have h2s.
- the cards don't need an extra wrapping div. Keep the html as simple as possible.
- for the top content give it a max width in rem so it scales correctly for all users no matter what their text size settings. This would be instead of setting an explicit width in px.
- get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
- if making the body into a flex container don't forget to set its direction to column.
- Font size must never be in px. This is really important! All of the font sizes look too small in this compared to the design too, as has already been mentioned.
- the cards should not have a width. Their width will be controlled by the grid template when you add it and be 1fr (the grid itself should have a max width in rem).
- do not add hover effects to non-interactive elements! That would imply interactivity but there is nothing interactive shown here. -'the icons should have margin left of auto not an explicit px value.
- mobile styles should be the default. You should then be adding a min-width media query defined in rem or em at the point where there is room for the layout to change. See https://fedmentor.dev/posts/responsive-meaning/ for more info.
0 - @yosefHeshamPosted 6 months ago
well done doing this design. notes:
- consider use semantic tags like <header> and <section>
- font size is smaller than the design
0@grace-snowPosted 6 months ago@yosefHesham there is no header shown in this design and no content that would be appropriate to use a header.
Using section elements for the cards would be fine but brings no additional benefits at all in this.
The important thing is the page has included content inside a main landmark, which is essential!
0@yosefHeshamPosted 6 months ago@grace-snow I think the title might be an appropriate content to be used inside header.. or maybe not. Anyways, the feedback is not attacking you personally, just a matter of opinions. Have a nice day :)
0@grace-snowPosted 6 months ago@yosefHesham it's really not a matter of opinion on this one. The header element is a landmark with a "banner" role. That means it is for repeating content across a site (like a logo or navigation). It must never include page-specific content like a h1. It's actually very important for accessibility and SEO.
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