Skip to content

Develop#2727

Open
Chalaczi wants to merge 15 commits intomate-academy:masterfrom
Chalaczi:develop
Open

Develop#2727
Chalaczi wants to merge 15 commits intomate-academy:masterfrom
Chalaczi:develop

Conversation

@Chalaczi
Copy link

No description provided.

.footer {
width: 100%;
height: 200px;
background-image: url(../../images/footer\ pic.jpg);

Choose a reason for hiding this comment

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

  • popraw ścieżkę do obrazka i usuń dodawanie go za pomocą img aby nie duplikować
Suggested change
background-image: url(../../images/footer\ pic.jpg);
background-image: url(../images/footer\ pic.jpg);

Comment on lines +197 to +201
</section>
<footer class="footer">
<img src="./src/images/footer pic.jpg" alt="footer bike">
</footer>
</main>

Choose a reason for hiding this comment

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

to img ci poszerzało layout bo nie ustawiłeś mu konkretnej szerokości. możesz usunąć img bo i tak dodajesz ten obraz jako background image a footer lepiej mieć poza kontenerem main aby na background image nie wpływał padding

Suggested change
</section>
<footer class="footer">
<img src="./src/images/footer pic.jpg" alt="footer bike">
</footer>
</main>
</section>
</main>
<footer class="footer"></footer>

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

  • deploy your project using npm run deploy command and add link to demo in pr description
image
  • adjust background image position so it matches design
  • phone number should be visible only when hovering over phone icon
  • navigation has too much horizontal spacing because two paddings are applied (one on header and one on navigation)
  • burger menu icon is too small
  • title is positioned slightly too low
image
  • close icon should be positioned on the right
  • list of links starts too low
  • spacing between links is too large
  • the last two links shouldn't be on the same line and should be positioned higher

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

deploy your project using npm run deploy command and add demo link to pr description

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

  • logo oraz ikony menu powinny być na tej samej wysokości aby dało się bez zmiany położenia kursora otwierać i zamykać menu
Image
  • musisz dodać jeszcze breakpointy oraz stylowanie dla wersji tabletowej
Image

pamiętaj o CHECKLIST:

  • Add a favicon
  • Change text color on hover for phone, email and address
  • When you click on phone icon or phone number in contacts section, make sure that there is no 404 error, make it a real link to start a call on device
  • Pictures in Gallery should increase on hover
  • Location-related addresses / links should open google maps in a new tab target="_blank"
  • Form shouldn't be submitted if some of the fields are not filled
  • disable page scrolling under the menu

.footer {
width: 100%;
height: 200px;
background-image: url(../images/footer\ pic.jpg);

Choose a reason for hiding this comment

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

zmień nazwę pliku z tej ścieżki pozbywając się spacji bo zdjęcie ci się nie pokazuje

Choose a reason for hiding this comment

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

zmień nazwę pliku z tej ścieżki pozbywając się spacji bo zdjęcie ci się nie pokazuje

Image

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

za każdym razem gdy commitujesz zmiany musisz zaktualizować demo link poprzez ponowne odpalenie komendy npm run deploy

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

moje komentarze naprawią ci stylowanie wersji mobile i desktop. musisz jeszcze zaimplementować wersję tabletową.

background-size: cover;
background-position: 90%;
min-height: 100vh;
padding: 0 20px 80px;

Choose a reason for hiding this comment

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

you already have margin bottom for title

Suggested change
padding: 0 20px 80px;
padding: 0 20px;

Comment on lines +16 to +18
&__title {
margin-right: 24px;
margin-bottom: 32px;

Choose a reason for hiding this comment

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

title shouldn't be centered and have all this additional margins

Suggested change
&__title {
margin-right: 24px;
margin-bottom: 32px;
&__title {
justify-self: flex-start;
padding-bottom: 32px;

font-size: 23px;
line-height: 140%;
letter-spacing: 0;
margin-bottom: 92px;

Choose a reason for hiding this comment

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

you already have padding for section

Suggested change
margin-bottom: 92px;

font-weight: 500;
font-style: Medium;
font-size: 32px;
padding-bottom: NONE;

Choose a reason for hiding this comment

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

invalid value

Suggested change
padding-bottom: NONE;
padding-bottom: 0;

@@ -0,0 +1,31 @@
.model {
&__photo {

Choose a reason for hiding this comment

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

add margin

Suggested change
&__photo {
&__photo {
margin-bottom: 32px;

.footer {
width: 100%;
height: 200px;
background-image: url(../images/footer\ pic.jpg);

Choose a reason for hiding this comment

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

zmień nazwę pliku z tej ścieżki pozbywając się spacji bo zdjęcie ci się nie pokazuje

Image

margin-bottom: 32px;
grid-column: 1 / 3;

@include mixins.desktop {

Choose a reason for hiding this comment

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

Suggested change
@include mixins.desktop {
@include mixins.desktop {
padding: 120px 120px 0;


@include desktop {
&__form {
grid-column: 1 / 7;

Choose a reason for hiding this comment

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

Suggested change
grid-column: 1 / 7;
grid-column: 1 / 6;

Comment on lines +12 to +20
&__description {
font-family: Poppins, sans-serif;
font-weight: 400;
font-style: Regular;
font-size: 16px;
line-height: 140%;
letter-spacing: 0;
margin-bottom: 16px;
}

Choose a reason for hiding this comment

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

Suggested change
&__description {
font-family: Poppins, sans-serif;
font-weight: 400;
font-style: Regular;
font-size: 16px;
line-height: 140%;
letter-spacing: 0;
margin-bottom: 16px;
}
&__description {
font-family: Poppins, sans-serif;
font-weight: 400;
font-style: Regular;
font-size: 16px;
line-height: 140%;
letter-spacing: 0;
margin-bottom: 16px;
@include desktop {
width: 70%;
}
}

Comment on lines +3 to +13
.header {
background-color: rgb(0, 0, 0);
background-image: url(../images/Cover_photo.jpg);
background-repeat: no-repeat;
background-size: cover;
background-position: 90%;
min-height: 100vh;
padding: 0 20px 80px;
box-sizing: border-box;

grid-template-rows: 1fr 1fr;

Choose a reason for hiding this comment

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

lepiej jest centrować tło dla wersji desktop

Suggested change
.header {
background-color: rgb(0, 0, 0);
background-image: url(../images/Cover_photo.jpg);
background-repeat: no-repeat;
background-size: cover;
background-position: 90%;
min-height: 100vh;
padding: 0 20px 80px;
box-sizing: border-box;
grid-template-rows: 1fr 1fr;
.header {
background-color: rgb(0, 0, 0);
background-image: url(../images/Cover_photo.jpg);
background-repeat: no-repeat;
background-size: cover;
background-position: 90%;
min-height: 100vh;
padding: 0 20px 80px;
box-sizing: border-box;
grid-template-rows: 1fr 1fr;
@include desktop {
background-position: center;
}

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

  • element ma podwójny padding ponieważ na header nadajesz padding z wersji tabletowej a na nawigacji pozostaje nadal padding z wersji mobilnej:
Image
  • podobny problem z podwójnym paddingiem dla tytułu tyle że żaden z nich nie jest tym prawidłowym z wersji tabletowej:
Image
  • padding boczny dla main powinien być taki sam co w headerze dla wersji tabletowej
Image
  • sekcja compare bikes powinna mieć dodatkowe wcięcie w wersji tabletowej
Image
  • zdjęcia powinny mieć inne proporcje w wersji tabletowej
Image Image
  • zmiana układu dla sekcji contact us
Image
  • usuń domyślny border dla inputów w formularzu
  • zdjęcie z footera nie jest wyświetlane. zmień nazwę używanego zdjęcia w taki sposób aby nie było tam spacji

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