-
Notifications
You must be signed in to change notification settings - Fork 91
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
Created a frist webpage - project 1 Ivana #56
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.
Hi Ivana!
Thanks for sharing your project, it was really nice to go through. Your code is quite clean and the flexbox is working out great. Keep up the good work 👍
<header> | ||
<div class="Header1"> | ||
TRADITIONAL FOOD | ||
</div> | ||
<div class="Header2"> | ||
MADE WITH LOVE & LOTS OF VEGETA | ||
</div> | ||
</header> | ||
|
||
<menu> | ||
<a href="./">Home</a> | ||
<a href="./recipes.html">Recipes</a> | ||
<a href="./howto.html">How To</a> | ||
<a href="./Contact.html">Contact</a> | ||
</menu> |
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 should be inside the <body>
tag. And remember to have consistent indentations
<div class="Flex-container"> | ||
<div> | ||
<p class="Abschnitt"> |
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 a minor thing but class names should have lower case letters and always be in English :)
img { | ||
width: 450px; | ||
height: 500px; | ||
} |
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.
When you have fixed width and height on images they might get distorted if it doesn't fit with their original ratio. Add object-fit: cover;
to fix this.
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.
Another thing about images - for better performance and to prevent slow loading is to make the images smaller (there are tools for sizing them down to the size they will appear on your page)
menu a:hover { | ||
background: rgba(13, 105, 58, 0.518); | ||
color: rgb(49, 49, 49); | ||
} |
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 with a hover effect 👍
header { | ||
background-image: | ||
url("https://images.unsplash.com/photo-1660634806611-d3d03b7c28fe?ixlib=rb-1.2.1&ixid=MnwxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8&auto=format&fit=crop&w=2070&q=80"); | ||
background-size: cover; | ||
height: 300px; | ||
padding-top: 150px; | ||
} |
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.
Really nice looking hero image! Maybe a shadow on the text would make it easier to read?
Thank you in advance for a first feedback.
Please note, that I'm not yet finished.