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

task solution #2677

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

Conversation

KrawczykDamiann
Copy link

@KrawczykDamiann KrawczykDamiann commented Jan 17, 2025

[DEMO LINK] https://KrawczykDamiann.github.io/layout_miami/

Nie ładuje styli! Problem ze strony GH.

Copy link

@danon321 danon321 left a comment

Choose a reason for hiding this comment

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

Link do demo nie działa :c

Jak wpisuje taki tez: https://krawczykdamiann.github.io/layout_miami/

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.

  • styles in your demo don't work so try to make deploy again
  • when switching from menu to the details, images overlap the menu background:
    image

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
  • When clicking on any location / address - prevent errors and make it to open location in Google Maps
  • Pictures in Gallery should increase on hover
  • Placeholders in the forms suggest what to enter; apply validation of the form fields (required, email / tel etc.), then it is clear in what format to enter the data
  • Form shouldn't be submitted if some of the fields are not filled

Copy link

@danon321 danon321 left a comment

Choose a reason for hiding this comment

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

Style ani obrazki dalej sie nie ładuja. Wychodzi na to ze sa jakies problemy ze ścieżkami.

Jak sam nie ogarniesz wbij na jakies QnA to ktos na pewno ci pomoze

Copy link

@danon321 danon321 left a comment

Choose a reason for hiding this comment

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

Ogólnie super robota!

Jest tylko mały szczegół

Tutaj masz zdjecie z figmy
image

tam szerokosc całego skrinu jest na jakies 1260px a zauwaz ze kontent i tak nie styka sie z krawedziami ekranu.

A tutaj skrin z mojego monitora 1980px
image

Radze dodac jakis container tak aby trzymał on kontent wysrodkowany i w odpowiednich ryzach. Sprobuj uzyc max-width margin auto

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.

  • above 1260px you have white background on sides ;)
    image
  • disable page scrolling for menu because now you can see images:
    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.

  • disable page scrolling for menu because now you can see images:
    image

you still need to disable scrolling for menu. suggestion on how to do it you can find inside
checklist

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.

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.

  • your padding for entire page is wrong. it's always 20px and it should be 120px for desktop, 72px (section compare bikes centered) for tablet
    image
    image
  • change color for email, address, phone and add hover effect. fix textarea
    image
  • you don't have close button for menu
    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.

Copy link

@danon321 danon321 left a comment

Choose a reason for hiding this comment

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

Nie widze zmian o ktorych mówiła Natalia wczesniej

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.

  • popraw layout dla tabletu (przejdź po wszystkich elementach i sprawdź czy zgadzają się odległości bo też np nawigacja powinna mieć większy padding górny więc zobacz czy gdzieś jeszcze są tego typu błędy)
    image
  • cała sekcja compare bikes ma większe marginesy oraz zbyt duży odstęp między nazwami a paragrafami
    image

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.

3 participants