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

Solution Inga Tkachuk #1884

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Solution Inga Tkachuk #1884

wants to merge 12 commits into from

Conversation

IDWIL8
Copy link

@IDWIL8 IDWIL8 commented Dec 18, 2024

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's improve your page

  1. Add the horizontal padding for the content
image
  1. Add a favicon on the page
image
  1. Where is the phone icon?
image
  1. Add transition when this menu appears on the page and remove scroll when menu is open
image
  1. Where are the second images?
image
  1. The user cannot be able to change size of the textarea field
image

@IDWIL8
Copy link
Author

IDWIL8 commented Dec 19, 2024

@volodymyr-soltys97 Thanks for the review! I've resolved all the issues.

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

please review your demo link again and compare it with the Figma design; they should look very similar. If you're facing any issues with the implementation, don't hesitate to ask for help in the chat

@IDWIL8
Copy link
Author

IDWIL8 commented Dec 20, 2024

please review your demo link again and compare it with the Figma design; they should look very similar. If you're facing any issues with the implementation, don't hesitate to ask for help in the chat

Hi, there is widespread comment, please tell me what you actually mean with examples.

@IDWIL8 IDWIL8 requested a review from etojeDenys December 20, 2024 04:41
Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

  1. remove the default body margin
image
  1. you should limit the content and center it in container
image image
  1. fontsize should be 16px
image
  1. looks different
image image
  1. let's fix small jumps on fields hover
  2. should be links
image
  1. looks different
image image
  1. you should stretch the button
image

@IDWIL8
Copy link
Author

IDWIL8 commented Dec 23, 2024

@etojeDenys Wow, thanks a lot for the review, it makes me better) I fixed all the issues you pointed that

@IDWIL8 IDWIL8 requested a review from etojeDenys December 23, 2024 11:46
Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 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.
Needs some improvements.

  • suggest to limit max content width. 1440px will be good (on wide screen now some images visible on full screen)
    Screenshot 2024-12-23 at 14 25 36
  • add paddings for inputs
    Screenshot 2024-12-23 at 14 25 16
  • add cursor pointer for all interactive elements.
  • add transition for all elements with hover effect
  • click on phone should start a call (check in header)
  • click on address should open google maps in new tab.
  • nothing happened after click on submit button in form.

pay attention on checklist, check and fix points, which need it.

@IDWIL8
Copy link
Author

IDWIL8 commented Dec 24, 2024

@VitaliyBondarenko1982 Thank you for review; I hope I understand you correctly and did the right fixes

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

Much better, almost done.

  • content should be centered horizontally. also will be better scale only images in recommended section.
Screenshot 2024-12-24 at 13 13 35
  • compare contacts section with design. pay attention on vertical spaces between elements.
Screenshot 2024-12-24 at 13 14 06

@IDWIL8
Copy link
Author

IDWIL8 commented Dec 24, 2024

@VitaliyBondarenko1982 I fixed this "also will be better scale only images in the recommended section.", but this comment "content should be centered horizontally." a bit confused me I already have horizontal centered in my styles, maybe you can a little bit explain?

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

please, could you create the thread in the chat and ping me there, let's discuss there how to limit and center the block

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.

4 participants