CSS Grid & Flexbox Homework#12
Conversation
| <div>Blog</div> | ||
| </div> | ||
| </aside> | ||
| <div class="box d"> |
There was a problem hiding this comment.
you could use main tag here
| </head> | ||
| <body> | ||
| <header class="wrapper"> | ||
| <div class="box a">LOGO</div> |
There was a problem hiding this comment.
never use unclear classes: "a b c d e f d" classes have no sense and it is very difficult to read such code
| <link rel="stylesheet" href="index.css"> | ||
| </head> | ||
| <body> | ||
| <header class="wrapper"> |
There was a problem hiding this comment.
all content inside header tag is not correct. Create a simple div with class wrapper, inside which you will have all content (header, aside, main, footer)
| @@ -0,0 +1,225 @@ | |||
| html,body { | |||
| margin: 10px; | |||
There was a problem hiding this comment.
Never set margin to body. It will lead to layout height issues. Better set it to 0; It is not a good practice to set layout styles in body. There could be font styles or other styles that should be nested by all children. But not layout specific styles.
| background-color: #ffeead; | ||
| } | ||
|
|
||
| .wrapper { |
There was a problem hiding this comment.
Better add padding to wrapper instead of margin in body
| } | ||
|
|
||
| .box .box { | ||
| font-size: 1em; |
There was a problem hiding this comment.
what is the sense of setting 1em font-size here? You already have font-size 150% for all .box elements.
1em of 150% is still 150%. Remove it
|
|
||
| } | ||
|
|
||
| @media all and (max-width: 615px) { |
There was a problem hiding this comment.
why you have @media rules for 615px and for 652px? Better create one breakpoint for tablet size screens.
You could read this article (https://medium.freecodecamp.org/the-100-correct-way-to-do-css-breakpoints-88d6a5ba1862) to get more info about breakpoints
| } | ||
| } | ||
|
|
||
| @media all and (max-width: 652px) { |
There was a problem hiding this comment.
Check how does your page look like on small devices (320px width).
It breaks. Please fix this issue
|
|
||
| @media all and (max-width: 652px) { | ||
| .e { | ||
| background-color: #e67b90; |
There was a problem hiding this comment.
Why do you set up same background color? It was already specified above. Do not repeat yourself
|
|
||
| @media all and (max-width: 1192px) { | ||
| .d{ | ||
| background-color: rgb(255, 182, 86); |
There was a problem hiding this comment.
Do not repeat the styles that were already set up before.
| background-color: #ffeead; | ||
| } | ||
|
|
||
| .wrapper { |
There was a problem hiding this comment.
Make your wrapper take the full space of the screen.
| grid-column: 2/ -1; | ||
| } | ||
|
|
||
| aside { |
There was a problem hiding this comment.
Align the children properly (according the example)
| } | ||
| } | ||
|
|
||
| @media (min-width: 320px) and (max-width: 480px) { |
There was a problem hiding this comment.
On small devices your nav blocks are too high. The content allows you to make their height less and => save some space for important content on the screen
2 participants
No description provided.