You must never use px for font size. You must always use a responsive unit (rem)
This looks ok but all the font is a bit small. I’m not sure why you’ve gone smaller than guided in the style guide
You also want to make sure the button and input are the same size when next to each other. You should be able to do that with flex properties and maybe a height 100% on the button
Never limit the body width either
I’m surprised to see any absolute positioning on this… there’s a few odd looking styles to me. Maybe I need to look on my computer so I can inspect and understand the reasoning on some of this.
Marked as helpful
In html
- you are missing a form element
- you must label inputs
- error message wrapping elements should be programmatically linked to their input with aria-describedby. They should also have an aria-live attribute so the browsers knows to watch them for changes
- once this is a form your js will be simpler because you will be listening for submit and using the form data (you won’t need to get the email input separately)
- overall js could be simpler. Consider toggling one class instead of multiple on all different elements (eg on the form and use that one class as your styling hook)
- header doesn’t go inside main. By doing that you are removing all semantics. Main header and footer are all distinct landmarks
- instead of calling all font awesome icons, just download the 3 svgs you need
- it’s more performant to link fonts in the document head than import them in css
Marked as helpful
@User9511
Posted
@grace-snow Thanks for the feedback, I'm going to re-work the layout on this and try to update the above changes you've suggested.
