Design comparison
Solution retrospective
Can you please give me some feedback, would be greatly appreciated.
Community feedback
- @pikapikamartPosted about 3 years ago
Hey, great work on this one and congrats to your first challenge here in FEM^^. Like what Olesia Kissa said, the desktop layout is missing both the background-color and background-image, checking that one out would great.
Some other suggestions besides Olesia Kissa's awesome feedback:
- It would be great to have a base styling of this:
html { box-sizing: border-box; font-size: 100%; } *, *::before, *::after { box-sizing: inherit }
This way, handling an element specially its size will be easier because of the
box-sizing
- Don't use
position: absolute
a very large element. If you inspect your layout, hover on eitherhtml
orbody
tag, you will notice that it has no height since its element is beingabsolute
. Since you are just using this to center the layout, it would be much better to do it this way. But first, remove these stylings on the.container
:
position top left transform
Then on the
body
tag use these stylings:align-items: center; display: flex; justify-content: center; min-height: 100vh;
- The vector/illustration on this is just decorative. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if you are usingsvg
instead ofimg
tag. - Only use descriptive
alt
on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users. article
is not needed on this one. You typically usearticle
when the content can be distributed in different sections or pages since the content is independent on its own. Change it to usingp
tag.- Also, you don't need to use
br
on those to make each lines wrap in another row. You can just increase thepadding
on it so that it will limit the size of the text, making text go in another row if needed. The left and right side ofpadding
. - The music-icon as well is only decorative so hide it using the method above.
- Avoid using
id
to target and style an element since it is a bad practice due to css specificity. Instead, just useclass
to target element. - The
annual plan
and the pricing text below it are two separate text. For that one, you can use heading tag like anh2
on theannual plan
since it gives information about the section's content and usep
tag on the pricing. - Lastly, just adding the
background-color
andbackground-image
for the desktop layout.
Aside from those, great job again on this one.
0@simon-baumhauerPosted almost 3 years ago@pikapikamart Thank you so much for this thorough answer, I try now to implement all your corrections. I'm so surprised by this effort to help me. Thank you!
1 - @olesiakissaPosted about 3 years ago
Greetings, @simon-baumhauer! The markup looks good, however the styling needs some fixes:
- I would recommend you to check your body
background
property on both layouts because the color disappears once the viewport is > 375px and also the background url seems to look for the svg pattern file that's not in your project folder. You should update your repository with the corresponding images and I think it should work fine. - Your annual plan container
background
color isn't the one specified in the style guide, make sure to check out the codes for the color.
Good luck on your next proejcts :)
P.S. check out the
title
tag in your html file, if you leave it as empty it may cause issues during validations.0 - I would recommend you to check your body
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