Design comparison
Solution retrospective
What is your way to organise CSS code and what would you recommend me to change in my style.css file? Do you think the way I used div tags and flexbox properties is optimal for this challenge or is there a better way to do it?
Community feedback
- @grace-snowPosted almost 3 years ago
Hi Anna, Well done on your first challenge! This is the time to build those foundational good practices that will stay with you long term.
Feedback for html
- use landmark elements. That means
main
for the container andfooter
for attribution - I wouldn’t use article for this component, but you can if you want.
- more important is to use a heading on this. The accessibility report will warn you this should be a h1 because every page has to have one, but as this isn’t a full web page, I’d probably use h2 for this component.
- images have to have an alt attribute. In this case the image is really important, so the alt needs to contain a description like “QR code”
- there’s nothing wrong with wrapping the img in a div, but it shouldn’t be necessary. You can set images to be display block in css
I’ve not got time to feedback on css right now but will do soon
Marked as helpful0@Anq92Posted almost 3 years ago@grace-snow Thank you for the feedback, it's really clear and helpful!
0@grace-snowPosted almost 3 years agoHere are some notes on changes I'd recommend to css and why. I made them quickly in browser dev tools, so please excuse th formatting
html { font-size: 62.5%; note: Strongly recommend against doing this. Resizing your html base font size can cause huge accessibility issues for people who rely on different font/zoom settings. There's no need to make 1rem equal 10px, let it be the default. } body { margin: 0; note: include a small css reset at the start of every stylesheet eg Reset by Josh Comeau or Andy Bell. This will make all browsers treat elements the same and remove things like margin from the body.; } .box { /* height: 500px; */ /* width: 320px; */ /* margin: 0 auto; */ max-width: 30rem; margin: 1rem; padding: 1.8rem; note: try to use rem weherever you're tempted to use px. It doesn't have to be exact for these challenges but is a good habit to get into as early as possible. Tweak in browser dev tools until it looks about right; note: don't add height to elements unless you actually need to. Let the component grow if it needs to. note: max-width is better than width for this because it allows the component to shrink if needed on smaller screens without causing overflow/horizontal scroll. note: margin 1 rem is to stop the component hitting screen edges on small screens. } .img-setting { /* height: 287px; */ /* width: 287px; */ /* margin: 17px; */ display: block; max-width: 100%; note: no need for width or height on this } .text-container { /* position: relative; */ /* margin: 0px 0px; */ /* width: 250px; */ } p.main-text { /* margin: 5px 0px 10px 0px; */ note: why is this css selector more specific? Keep specificity low, to single classes if possible note: must be a heading; } .main-text { /* letter-spacing: 0px; */ margin: 1.6rem 0; note: letter spacing should be unitless, not px. When letter spacing is 0 that's already the default so this line can be removed.; } p { margin: 1.6rem 0; }
Marked as helpful0 - use landmark elements. That means
- @JanselinPosted almost 3 years ago
Hey there! Congrats on your solution! you did great job! I think you did very well with the use of
<div>
. Here some tips!🔹I would recommend you to use a
<article>
tag for thediv class="box"
🔹 you could replace
<div class="container">
for a<main >
tag.🔹
<p class="main-text">
should be a<h1>
Just for a better html structure.
Happy coding 🤗
Marked as helpful0
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