-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Julia's project 2 #23
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really impressed by your code. 😊 It's easy to follow, with great naming and nice, clear structure. You've used semantic HTML-tags, and a correct hierarchy. Really good job!
I left some tips & tricks throughout the code, because there were very few things that needed fixing. 🌟🌟 Keep up the good work!
@@ -0,0 +1,97 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there! 😊 I'm Karin, and I will review your code today.
</head> | ||
|
||
<body> | ||
<header> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see that you are using semantic HTML-tags. Great work! 👍
<h1>Anna Leutert</h1> | ||
<h2>Handweberin, Textil- und Farbdesignerin</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also very good. Using a correct hierarchy in titles/text-tags makes your site better according to Google. Meaning your SEO is on point! 🙂
<p>Ich bin Mutter von 4 tollen Töchtern. </p> | ||
</div> | ||
<!--Trennlinie--> | ||
<hr class="solid"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hr-tag is not used that often nowadays, because of it's lack of customization options. A <span>
-tag would give you a lot more options if you want to add a visual element to separate your sections. For example, you can use borders, width and height, and even gradients and drop shadow. 🎨😄
|
||
<footer> | ||
<!-- newsletter --> | ||
<section class="newsletter-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is an overall thought: GREAT naming of your tags. You can easily follow your code and understand what divs contains what content, and what they're for. Keep up the good work! 🌟
padding: 8px 10px; | ||
text-decoration: none; | ||
border-radius: 5px; | ||
} | ||
|
||
body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually you structure the css-file starting from the outside and top. So the body would usually go first, at the top of the document 😊
} | ||
|
||
|
||
body h2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying a h2 within the body is basically the same thing as writing only h2, as the body is the wrapper of all content. 😊
} | ||
|
||
.photo { | ||
padding: 10px 10px 10px 10px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want the same amount for all directions, as 10px here, you could simply write padding: 10px
, as it will apply the same amount to all sides. If you want to add different to top and bottom, you could write padding: 20px 10px
, and it would get 20px on top and bottom, and 10px on left and right. 🌈
|
||
|
||
|
||
/** MEDIA QUERIES **/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you've added all media queries to the bottom of the document! 👍🏻
|
||
/** MEDIA QUERIES **/ | ||
|
||
@media (min-width: 650px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also great that you've work mobile first! That's best practice, and a really good thing to get used to! 😊
In this project I learned about the media queries and played around with color.