@thomashertog
Posted
Your solution is looking good visually, there are some improvements that can be made though.
HTML
- I feel you're using the heading levels wrong. If you'd write them out in a table of contents like this
1. <h1> / 1.1. <h2> / 1.1.1. <h3>it doesn't really make sense - The
<button>is missing atype - The content in the Why us section feels like a list of links to me, so I'm wondering why you marked it up as a
<p>with<br>
CSS
- no need to make the button fixed size in both
widthandheightusingpx - the code within the media query for your
bodyis not needed in a media query, it should be the default (and making sure that the mobile view is vertically centered as well) - a lot of the properties don't affect anything in particular, so you could easily save a few lines of code here and there (making it easier to change afterwards when necessary)
Marked as helpful
@anespoul34
Posted
@thomashertog
Hi Thomas, thanks for the feedback.
I feel the same for headings. Since there are 3 sections, One major section (community) with h1 and h2, and then 2 minor sections (subscription and why-us) with h3, I felt like it would make sense. How would you manage it? using <p> with classes ?
For the why us section I just tried to match the Figma and the padding wasn't giving me the expected result so I just used <br>
@thomashertog
Posted
@anespoul34 this is my solution. I went with an h1 that is screenreader-only to give some more context to the page, h2 for Join our community and then wildly inconsistent (so will need to change that) a header (with h3) for monthly subscription and why us
you can also take a look at the list of links to see how the padding/margin works out.
I don't have the figma file so it's more of a resembling look and feel than an "exact" copy of the design of course
Marked as helpful
