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
mainfor the container andfooterfor 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 helpful
0
Anna• 130
@Anq92
Posted
@grace-snow Thank you for the feedback, it's really clear and helpful!
0
Here 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 helpful
0
