finish first page #65
Conversation
There was a problem hiding this comment.
Well done! I'm impressed by how far you've got already and how neat your design looks. You can really be proud of yourself 🥳
You have a solid html structure and your stylings are well organised as well, although you could simplify in a few places. I wasn't sure if the relevant index.html was the one in the code folder or the other one. If it's the one in the code folder, I'd suggest you remove the duplicates to avoid confusion :) My comments should apply to both of the index.html and the style.css files. If anythings unclear you can just re-comment or we'll take a look next Saturday :)
| /* Here in the css file you'll write your styling. make sure to link the css file to the html file. If it's linked correctly you should see a light blue background. */ | ||
|
|
||
| body { | ||
| background-color: white; |
There was a problem hiding this comment.
If you'd like to get rid of the margin that's applied by default in the body, you could add margin: 0; here
| .menu { | ||
| font-family: "Arial"; | ||
| text-transform: uppercase; | ||
| font-size: 12pt; | ||
| display: flex; | ||
| background-color: #00311a; | ||
| color: #eeeeee; | ||
| text-align: center; | ||
| padding: 20px; | ||
| cursor: pointer; | ||
| margin-top: 10px; | ||
| margin-bottom: 10px; | ||
| flex-direction: row; | ||
| justify-content: space-between; | ||
| } |
There was a problem hiding this comment.
Because the heading1 has a large margin per default, the margins of the menu don't have any effect, so you could safely remove them. You could also remove text-align: center; because you've positioned the links with flexbox. Also, flex-direction: row; is set by default, so you don't have to specify it. Setting justify-content: space-around; would also look nice if you'd like to try :)
So, I'd suggest the following changes:
| .menu { | |
| font-family: "Arial"; | |
| text-transform: uppercase; | |
| font-size: 12pt; | |
| display: flex; | |
| background-color: #00311a; | |
| color: #eeeeee; | |
| text-align: center; | |
| padding: 20px; | |
| cursor: pointer; | |
| margin-top: 10px; | |
| margin-bottom: 10px; | |
| flex-direction: row; | |
| justify-content: space-between; | |
| } | |
| .menu { | |
| font-family: "Arial"; | |
| text-transform: uppercase; | |
| font-size: 12pt; | |
| display: flex; | |
| background-color: #00311a; | |
| color: #eeeeee; | |
| padding: 20px; | |
| cursor: pointer; | |
| justify-content: space-around; | |
| } |
| <a href="overview.html">Welcome</a> | ||
| </div> | ||
| </div> | ||
| <div class="footer"> |
There was a problem hiding this comment.
Have you considered adding these links to the menu above? It would be more useful to scroll to the sections they link to. Right now, the section is right below so the scrolling doesn't really work.
| <meta charset="utf-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>FoodBlog</title> | ||
| <link rel="stylesheet" type="text/css" href="style.css" |
There was a problem hiding this comment.
| <link rel="stylesheet" type="text/css" href="style.css" | |
| <link rel="stylesheet" type="text/css" href="style.css" /> |
| margin-bottom: 10px; | ||
| flex-direction: row; | ||
| justify-content: space-between; | ||
| scroll-behavior: smooth; |
There was a problem hiding this comment.
If you'd like to have smooth scrolling, you'd have to set it in the html tag:
html {
scroll-behavior: smooth;
}
| text-decoration: none; | ||
| } | ||
| .welcome { | ||
| margin-top: 15pt; |
There was a problem hiding this comment.
pt is usually only used in print to make sure the font has exactly the right size. In web development and for your websites specifically, I'd recommend you use px for spacings such as margins or paddings and also font sizes.
| .aboutme { | ||
| margin-top: 12pt; | ||
| font-family: "Arial"; | ||
| text-align: center; | ||
| color: #00311a; | ||
| } | ||
| .contact { | ||
| margin-top: 12pt; | ||
| font-family: "Arial"; | ||
| text-align: center; | ||
| color: #00311a; | ||
| } |
There was a problem hiding this comment.
The cool thing about classes is that they're reusable. So if two elements should have exactly the same properties, you can just set the same class in the html. So here you could remove one of them and reuse the other (see other comment)
| <a href="#aboutme">About Me</a> | ||
| <a href="#contact">Contact</a> | ||
| </div> | ||
| <h2 class="aboutme" id="aboutme"> |
There was a problem hiding this comment.
| <h2 class="aboutme" id="aboutme"> | |
| <div class="aboutme" id="aboutme"> |
| inspriration with me. | ||
| </p> | ||
| </div> | ||
| <div class="contact" id="contact"> |
There was a problem hiding this comment.
Because the contact class has the same properties as aboutme, you could remove contact and set here aboutme instead (maybe it would make sense to rename the class to something more general):
| <div class="contact" id="contact"> | |
| <div class="aboutme" id="contact"> |
| .splitview-inner { | ||
| display: flex; | ||
| flex-direction: row; | ||
| flex-wrap: wrap; |
There was a problem hiding this comment.
If you don't specify a width for the instructions, it'll also wrap on desktop because the two elements don't fit beside each other. If you'd only like to wrap on mobile, you could set flex-wrap in the media query below
|
Hi Nadia, thx for your time and all good proposal! I removed all redundant code and will keep eye on this (and not only focus on coding haha). I have one question that I will ask you on Saturday |
No description provided.