Design comparison
Solution retrospective
Feedback will be appreciated as per!! π
Community feedback
- @dwhensonPosted about 3 years ago
Hey @Tomi-pter nice work on this one!! π The page looks great! π
Here's some thoughts you might like to consider:
β’ You should probably use a
nav
rather than adiv
for the links in the header, and I think you should definitely use it for the links in the footer as this is clearly a navigation section of the page.β’Β I would suggest having a think about your overall approach to page layout and how you position the overall elements on the page. You've a couple of approaches in this one page, with elements in the header and footer lining up nicely, and the elements in the main line up nicely with each other but not the header/footer.
Related to this is that the header and footer spread right accross the pade even at large viewports, which makes things too spread out. There are many ways to do this but I set a grid on the body element, with three columns, as using a class selector as follows:
.center-content { display: grid; grid-template-columns: minmax(1rem, 1fr) minmax(375px, 1440px)minmax(1rem, 1fr); } .center-content > * { grid-column: 2; }
The 1440px is the max-width you want the main content to be, and the 1rem values is the smallest spacing you want either side of the main content on small screens (I sometimes put this to 0 and use a container to add padding to each section).
The second part positions all direct children of the body in this nicely sized middle column. In my case, mostly, my header, main, and footer the middle column, and stops them going wider than 1440px. Itβs also pretty easy to βbreakβ elements out of this constraint if you need to.
Other people use container classes to do the same thing. This article has a good run down of alternative approaches https://css-tricks.com/the-inside-problem/ You will note I am actually using the approach the author doesn't like!
Either way it's a good idea to find an approach that works for you as you'll need this for a lot of FEM challenges. In this challenge you will then want to set the header and footer to
grid-column: 1 / -1
so they span the whole page, if you then wrap all content in a container in bot the header and footer, and add the center-content class to the header and footer you should be sorted!Sounds complicated, but it's not too bad if you play around with this. There's many other ways to do this, and they might be better and simpler!
Hope this helps and just let me know if anything is not clear. Keep up the good work!! π
Cheers π
Dave
Marked as helpful3@Tomi-pterPosted about 3 years ago@dwhenson Thanks for the suggestions. The css tricks article was really helpful. I used the first method he/she outlined as i found that the easiest to use and remember. I wrapped the contents of the 3 sections (header, main and footer) in a div (class = "wrapper") then i set the max width of .wrapper to 1440px and set the margin to auto to center it. Thanks once again ππΎ
1@dwhensonPosted about 3 years ago@Tomi-pter just had another peek! Good job! Looks great now!
0 - @MojtabaMosaviPosted about 3 years ago
Looks good, take a look at the followig things:
1- Limit the width of the cta in the header because it's to wide, a max-width and centering would be enought.
2- The header is not centered which makes the layout look a bit off, this becomes obvios once you approach larger screen around 1400px.
Keep coding :=)
Marked as helpful1@Tomi-pterPosted about 3 years ago@MojtabaMosavi Thanks for the feedback. I've carried out the corrections, they were really helpful (especially no2, I didn't even notice π ).
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