Skip to content
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

Vittoria's very first project /services website #60

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vittoriamatteoli
Copy link

I created this website for a company , I struggled with aligning the pictures but using the flexbox helped me .
I didn't link any new page on the menu but just linked the sections on the same page for now, I will add content later.
The contact form it's not functional but I just styled it with CSS , since we will get to it further in the lessons.
Logo and Favicon that I added might be not highest quality, I just wanted to learn how to insert them.
Looking forward for a feedback.

Comment on lines +21 to +33
<div class="container2">
<div class="logo">
<img src="logo2.png" alt="logo" />
</div>
</div>
<nav>
<a href="./index.html">Home</a>
<a href="#services">Services</a>
<a href="#contact">Contact us</a>
<a class="appointment-button" href="mailto:[email protected]"
>Book now</a
>
</nav>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using position: absolute for your header, I'd recommend using flexbox. You could wrap the nav in the container2 class and apply flexbox there. It would simplify your html and css (see other comment)

Suggested change
<div class="container2">
<div class="logo">
<img src="logo2.png" alt="logo" />
</div>
</div>
<nav>
<a href="./index.html">Home</a>
<a href="#services">Services</a>
<a href="#contact">Contact us</a>
<a class="appointment-button" href="mailto:[email protected]"
>Book now</a
>
</nav>
<div class="container2">
<img src="logo2.png" alt="logo" />
<nav>
<a href="./index.html">Home</a>
<a href="#services">Services</a>
<a href="#contact">Contact us</a>
<a class="appointment-button" href="mailto:[email protected]">Book now</a>
</nav>
</div>

Comment on lines +28 to +51
.logo {
float: right;
}
.container2 {
position: absolute;
top: 0;
clear: none;
overflow: hidden;
background-color: white;
}
nav {
float: right;
position: absolute;
top: 0;
right: 30%;
padding: 30px;
overflow: hidden;
background-color: white;
height: 80px;
word-spacing: 5px;
margin-left: 50px;
margin-bottom: 50px;
text-transform: uppercase;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With flexbox instead of an absolute position, you wouldn't need as many css properties and you could remove the logo class. Maybe try with the following:

Suggested change
.logo {
float: right;
}
.container2 {
position: absolute;
top: 0;
clear: none;
overflow: hidden;
background-color: white;
}
nav {
float: right;
position: absolute;
top: 0;
right: 30%;
padding: 30px;
overflow: hidden;
background-color: white;
height: 80px;
word-spacing: 5px;
margin-left: 50px;
margin-bottom: 50px;
text-transform: uppercase;
}
.container2 {
display: flex;
justify-content: space-between;
align-items: center;
background-color: white;
}
nav {
padding: 30px;
background-color: white;
word-spacing: 5px;
text-transform: uppercase;
}

</button>
</div>
</div>
<div id="services">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using display: inline-flex: for the containers, you could just add display: flex for the parent div (see other comment). Because you already set the width to 1/3%, all the containers would be positioned neatly in a row :)

Comment on lines +128 to +137
.container {
margin-top: 50px;
position: relative;
width: 33.33%;
height: 100%;
display: inline-flex;
flex-direction: row;
justify-content: center;
float: left;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add a .services class to the parent div of the containers, it would simplify your css.

Suggested change
.container {
margin-top: 50px;
position: relative;
width: 33.33%;
height: 100%;
display: inline-flex;
flex-direction: row;
justify-content: center;
float: left;
}
.services {
display: flex;
margin-top: 50px;
}
.container {
position: relative;
width: 33.33%;
height: 100%;
justify-content: center;
}


-->
<h1>Hello there! 👋🏼</h1>
<div class="icons">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could wrap the icon divs into a parent div and apply display: flex in the parent. This would make it easier in terms of responsiveness.

<div class="icons-wrapper">
  <div class="icons">...</div>
  <div class="icons">...</div>
  <div class="icons">...</div>
</div>

opacity: 1;
}

footer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the footer you could also apply display: flex to position the columns and make responsiveness easier in the next step.

Comment on lines +192 to +197
.footer-title1 {
text-transform: uppercase;
}
.footer-title2 {
text-transform: uppercase;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cool thing about classes is that you can use them in multiple places in your code. Because they have the same property, you could for example rename it to footer-title and use it for both titles in the html (see other comment)

<p class="footer-title">Choose the best cleaning services!</p>
</div>
<div class="column">
<h2 class="footer-title1">Adress</h2>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the same class to make it uppercase, e.g. footer-title

</a>
</div>
<div class="column">
<h2 class="footer-title2">Contact</h2>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the same class to make it uppercase, e.g. footer-title

@NadiaPosch
Copy link

I'm impressed by how far you've got in just a few weeks! The basic structure of your html looks very good. Parts of the CSS could be simplified or reused. I've made a few suggestions and comments but it's a great foundation for the second part of the course :) well done 👍

@vittoriamatteoli
Copy link
Author

Thank you so much for your feedback Nadia 😊😊 I will have a look of all suggestions !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants