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

Nav #747

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

Nav #747

wants to merge 19 commits into from

Conversation

katerinalex
Copy link

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.

Continue the conversation in the chat

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.

Looks like some sections not finished
Screenshot from 2023-08-31 12-37-19
Screenshot from 2023-08-31 12-37-00

Also:

  • do not show hamburger icon when visible navigation
    Screenshot from 2023-08-31 12-36-14
  • add hover effect for nav links. check other interactive elements.
  • after click on hamburger icon nothing happen. menu still closed
  • click on slider buttons leads to 404 page

Pay attention on checklist

Copy link

@loralevitska loralevitska 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! There are things that should be fixed:

  • fix 404 error by about us link click (same for nav links in footer)
  • make sure that all interactive elements have a cursor pointer on hover
  • would be great if something happens by click on nav links (same for nav links in footer)
    image
  • fix 404 error by carousel button click
    image
  • fix these blocks according design for desktop mode
    image
    image
    image
  • fix placeholder
    image
  • remove default autocomplete background color, it breaks design
    image
  • make all form field required
  • always open links in new tab
    image
  • add social links to appropriate icons (instagram, facebook, twitter)
    image
  • add smooth scroll to page by navigate, now user teleported after link click
  • fix menu. It doesn't work by click on burger menu icon

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.

Great job overall! Just couple tiny things needs to be fixed before approval)

  1. these links must lead to different part of page, and need to add hover on it
    image
  2. need to add hover on main button and remove default border style from it
    image
  3. menu button is not working
  4. these card need to make according design
    image
    image
  5. also this block
    image
  6. need to make according design
    image
  7. need to check it out
    image
    image
    image

Copy link

@loralevitska loralevitska 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! There are things that should be fixed:

  • center this block (need stretch)
    image
    image
  • fix footer according design
    design desktop mode
    image
    your version
    image
  • add right indents for social according design
    image
  • fix blocks width according design (264px)
    image
    your version
    image
    I'll help you with width and fix all in the card according design (indents between text and icons, font-size etc)
    image
  • add right paddings for these blocks
    image
  • add background and check indents according design
    your version
    image
    design version
    image
  • fix menu, it doesn't work
  • also check whole your page for tablet and mobile mode and fix according design (for example remove this line)
    image
  • there is no need open new tab phone number link
    image

If you have any questions, please ask them in fe_chat and tag mentors. It will be faster, because your answers in GitHub will be visible to mentor only after you re-request review

Copy link

@loralevitska loralevitska 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! need some improvements

  • check yourself with indents from figma for these blocks. Check each block on the page like this, and not only in the place where mentors left comments
    image
    image
  • according design background color should be white
    image
    image
  • cross icon should be the same place with burger menu icon
    image
  • add hover effect for menu links
    image
  • check yourself and fix all mismatches with design
    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.

  1. rewrite this button on div or remove porder and as i said before need to add hover effect
  2. need to centered this
    image
  3. this block is still not like in design
    image
  4. also here need to add hover
    image

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.

Something happened with your DEMO link :(
Fix it please
image

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.

Well done!

  1. It looks like not separated elements, small parts of final 'learn more button' i mean would be good if hover effect will be for zone with icon+text(and same) so text should be changed on hover on icon etc
    image
  2. items in list has too big margins
    image

mock:
image

  1. here should be bigger paddings
    mock:
    image

now:
image

  1. add max-w to this section and elements should be aligned with near sections vertically
    image

mock:
image

  1. add cursor pointer to form submit button

  2. footer should be aligned with other sections too, also looks like margin is lower than on mock(between form and footer)
    image

  3. remove underlining from location link
    image

@TarasHoliuk
Copy link

  1. Check margins between elements, and sizes in the footer (for different screen sizes):
  • App:

image

  • Mockup:

image

  1. The Instagram icon link doesn't change color on hover. Also, add transition for all elements with hover effect on the page (not inside :hover selector - directly for the element to make it work in both ways):

image

  1. Check the margin between the title and section content:
    image

  2. Fix the checklist points (and check all of them): 11, 12, 13, 15, 16

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

Good improvements 👍
Let's add more - pay attention to the checklist

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.

Great job overall! Just one tiny thing needs to be fixed before approval)

image
on mobile size this block without paddings

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.

You've got this, keep pushing forward!

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.

8 participants