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

implementation #736

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

implementation #736

wants to merge 10 commits into from

Conversation

NadieinOleh
Copy link

@NadieinOleh NadieinOleh commented Aug 4, 2023

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.

Please attached demo link to PR

Copy link

@Anastasiia-Svintsova Anastasiia-Svintsova 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! Check the design more thoroughly

  • Check the text position on the desktop. It doesn't match the design
    image

  • Where are the buttons?
    image

  • Nav links don't work. And add hover effects to them
    image

  • There shouldn't be a menu icon on the Desktop
    image

  • Use SVG for the logo for better image quality

  • The button should be rounded
    image

  • This hover effect looks strange in my opinion. Could you use a better approach to avoid overlapping?
    image

  • Cards don't match the design. And add a hover effect as well
    image

  • This button should be rounded, has a hover effect, and lead to some section.
    image

  • All external links should be opened in a new tab

  • There is no placeholder for the message
    image

  • Remove styles for autocomplete
    image

  • The form doesn't work correctly. It should be cleared after submitting

  • Add hover effects to these links
    image

  • The logo in the footer should lead to the top of the page

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.

The demo link isn't working now

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.

  1. Fix the issue with the slider, it should fit any display
Screen.Recording.2023-08-07.at.18.45.10.mov
  1. Check the design and add top padding to the section title
Screenshot 2023-08-07 at 18 46 19
  1. Input value shouldn't disappear when the user clicks on another input
Screen.Recording.2023-08-07.at.18.54.39.mov

Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Pls watch the video review :)

Attention.to.Design.Issues.mp4

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.

Nice work, but still have some bugs:

  1. in my screen i dont see gallery button at all
    image
  2. need to use same hover effects
    image
  3. this block is differ from design in mobile
    image
  4. menu button is not working
  5. on small mobile gallery is hidden a bit
    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.

Almost there) Just ones more thing needs to be fixed
1.
image
2. still have a different hover on it
image
3. menu button is not working

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.

Good work!

There are several things to improve.

  1. It's better to add cursor: pointer on hover any interactive elements. For example, buttons on your slider:
    image
    It's the best practice for user experience.

  2. I see you added a blue color on insta hover, but it should be the same behavior for any other social icons. It should be a general solution.
    image

  3. It may be something went wrong, but the mobile menu doesn't open on the menu click.
    image
    Could you check it, please?

Other things look good for me. Good job!

Copy link

@sTorba24 sTorba24 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

  1. Add hover effect on links(phone number, map) Also, remove target _blank from number
    image

  2. image should be sticked to right side
    image

now
image

  1. Add hover effects on card or card elements
    image

4.should be cursor pointer here
image

5.Menu still not working on mobile, fix it please

Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Did you forget to redeploy the project?) Previous comments aren’t fixed

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.

Good job!

You can improve a little bit and remove scroll for the whole page if your mobile menu is opened:
image

You can use overflow: hidden for the body if menu is opened and remove this rule is menu is closed.

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.

7 participants