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

Develop #643

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

Develop #643

wants to merge 11 commits into from

Conversation

IvanLugovskiy
Copy link

Copy link

@polosanya polosanya left a comment

Choose a reason for hiding this comment

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

Nice start! Keep up working!

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Good job!!!
but lets make some improves:

  1. hover on change theme buttons make other block jumping and other nav link haven't hover effects
    https://www.loom.com/share/8eb6cc5167944e348b0a7edfa8249398
  2. your container and break points are so huge
    image
    image
    3.add hover effect on phone number and browser must to initiate a call after click on it
    image
  3. user dont know what is must to fill in, add placeholder like in design and when i try to send a form, it must to clear they fields
    image
  4. footer link also must have hover
    image
  5. blocks must to take all available place
    image

@IvanLugovskiy
Copy link
Author

about third point, I can't understand why placeholder doesn't display after deploy( in local deploy it is successfully displays
image
image

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

You made a nice work, but still have to fix a couple of thingth
1.https://www.loom.com/share/d18d894061344331825b9b8f3c98b780
2. i think content must to take a bit more places
image
3.add link to home on logo icon
4 and would be better to add link on social icon (with target = _blanc)

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Almost good, but i still dont understand what i must to enter in form, please add placeholder for it
image

Copy link

@agniya-i agniya-i left a comment

Choose a reason for hiding this comment

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

Good job!

The carousel should display correctly on any display size

Screen.Recording.2023-08-10.at.12.02.58.mov

@IvanLugovskiy
Copy link
Author

The carousel should display correctly on any display size
Yes I understand, but I don't now how to make it, I have not yet learned js. Tell me please how to make it

Copy link

@witflash witflash left a comment

Choose a reason for hiding this comment

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

Finally great work!
Well done 👍

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.

5 participants