Hi
This looks good but there are some learning points particularly in the html
- there is no nav in the header of this. It's a logo and a link (not button). Headers only need to include a nav element if there is a navigation placed there in the design. The logo alt should ONLY say "Huddle" too
- headers have a role of "banner" and are only for repeating content across a site. They should never include page specific content like the h1. Move all of that out into main (so only the logo and "try it now" link are in the header)
- similarly the "get started for free" link needs to be an anchor (link) NOT a button. They have distinct purposes. Anchors are for navigation, buttons are for actions (like toggling content)
- you only need to use a figure element if using figcaption. Figures can wrap all sorts of content they are not generic image wrapping elements. It's not a "problem" using them, just pointing out that it brings no benefit and can add bloat to your html in some cases (ie if/when a wrapping element isn't needed)
- I recommend making the footer phone number and email into anchor links (optional but can be a user expectation for them to be clickable)
- the footer nav is really just one
ulnot two. Remember you can use css approaches like column properties or even grid to make the layout match the design on larger screens - social links should not be in a navigation element. They are external links, not a way to navigate this site
- the alt on the social images should ONLY say the name of the social platform. Don't over complicate accessible names. Assistive tech users usually hate unnecessary verbosity
Marked as helpful
In css (note this feedback is from reading the css file like a PR review would do - I am away from my computer so can't check the solution or css in browser)
- make sure you are defining media queries in rem/em not pixels
- Next time definitely work mobile first (it's probably a bit late to change on this challenge now)
- Never write letter spacing in px! You always want letter spacing to scale with the font size, so would be better using em (or even %) there. I don't actually think you need that property at all on this tbh
- instead of changing things like padding at each breakpoint consider using css functions like min() max() or clamp() so it can adjust automatically
- you don't need to repeat properties in a media query if they are the same as the default (eg your header only changes background image, none of the other properties change)
- use class selectors in css not element selectors - as well as keeping css specificity low (what you want for maintainable css!) it means that if you need to change the html elements (like I've recommended above) it won't break the styles as well. It is good to keep separate concerns
- you should never have line height 0. Did you mean to use line height 1 perhaps?
- when list style none is used in CSS some assistive technologies remove the list semantics altogether in the html. To restore this for all users, an easy way is adding role list to the ul/ol elements and role listitem to every li.
Marked as helpful
@SandipanIO
Posted
@grace-snow Thank you so much for reviewing my code! I really wanted someone to review my code and tell me what I was doing wrong.
Just one question - you asked me to move the hero section out of the Header area:
headers have a role of "banner" and are only for repeating content across a site. They should never include page specific content like the h1. Move all of that out into main (so only the logo and "try it now" link are in the header)
As per the design, the logo, "Try it free" link, and the hero section has the same background image. (In fact, most website designs follow the same principle)
So moving the hero section out of the header tag will affect the background image as I have added the background to the entire header area.
Can you please suggest a way of solving this problem?
P.S. - I have read some of your articles on your blog like whether we should use the 62.5% rem trick (I have been guilty of using it, switched to calc() now), the HTML structure article, etc. Learned so much from you, thank you.
@SandipanIO in this case you can apply the same background color to the header as what is in the hero
Alternatively there are pseudo elements
Or css grid
