@Anirog
@soitirakis
Submitted
Hello everyone. I tried to do this challenge as best as possible. If anyone can check the code quality, what can be improved? Can you check the correctness of the CSS file? There is any recommended structure or order to write from top to bottom the CSS ? Any other feedback is highly appreciated. Thank you
@Anirog
Posted
I read that media queries should always go at the bottom of CSS.
@soitirakis
Submitted
Hello everyone. I tried to do this challenge as best as possible. If anyone can check the code quality, what can be improved? Can you check the correctness of the CSS file? There is any recommended structure or order to write from top to bottom the CSS ? Any other feedback is highly appreciated. Thank you
@Anirog
Posted
Hi, your CSS looks really clean, I used a different approach to get the card exactly in the center of the screen using viewport height, viewport width and flexbox.
.container {
width: 100vw;
height: 100vh;
display: flex;
align-items: center;
justify-content: center;
}
There is any recommended structure or order to write from top to bottom the CSS ?
In CSS the code at the bottom overrides any styles above that style, so if you had:
h1 {
color: red;
}
h1 {
color: blue;
}
The h1 tag would be blue.
@bobbe86
Submitted
This challenge took me a couple hours to finish and I'm wondering..
What minor things would you change about the HTML and CSS layout and flow to follow best practice standards?
Did I use one too many divs? Not enough divs?
Should the container have a max-width of 375px?
Thanks in advance for your help!
@Anirog
Posted
I only used one div .container and then styled the elements in the .container like this:
.container { . . . }
.container img { . . .}
.container h1 { . . . }
.container p { . . . }
Not sure if this is best practice but it was easier for me to keep the index.html file small.
I think what you did using max-width: 375px; was better than what I did.
I just used width: 322px;
Your resets could be made simpler by using a wildcard selector * which styles everything so instead of:
/* Remove default margin */
body,
h1,
h2,
h3,
h4,
p,
figure,
blockquote,
dl,
dd {
margin: 0;
}
You could do this:
* {
margin: 0;
padding: 0;
box-sizing: border-box;
}
@helenborges201
Submitted
@Anirog
Posted
1 from the closing tag.<h1>Improve your front-end skills en by building projects</h>
Should be
<h1>Improve your front-end skills by building projects</h1>
h1
h1 {
padding: 0px;
}
Should be
h1 {
padding: 0px 20px 15px;
color: hsl(218, 44%, 22%);
}
p
p {
padding: 0px 20px 15px;
}
I'm not exactly sure if this the the best way to add padding but it looks more like the original design :)
Marked as helpful