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 #1016

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

Develop #1016

wants to merge 12 commits into from

Conversation

Vitalii120296
Copy link

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.

looks different
image
image

  1. should be the button with cursor pointer
image image

and it looks different too

  1. same here
image image
  1. this should be the links with some hover
image

@TarasHoliuk
Copy link

  1. Read the checklist to the task. Make sure you've reached all points

  2. Check font styles here (font-weight in particular):

image

  1. Remove default border:

image

  1. Check the font styles here:
    image

  2. The buttons should be full-width on the mobile version (instead of hardcoded width):
    image

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 improvement! But also, read the checklist

Added a few more comments to make your landing page look better in the portfolio in the future 🙂

@TarasHoliuk
Copy link

Great job! Almost done 🙂

  1. The nav links in the footer look different than in the Figma mockup:
  • Landing:

image

  • Figma:

image

  1. The comment about the blue button from the previous review isn't fixed. On the mockup, we see a mobile version example on a 320px width screen. But it should look the same on a 400px width mobile device. So just set the width 100% for buttons:

image

  1. Add hover effect to nav links and buttons (it will make landing page look more "alive") - that's not a part of current task checklist but good practice generally

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.

Almost done) check comments

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.

Almost done!
To improve:

  1. Add the topical address for these social networks and open them in the new tab
image
  1. Check the gap between the nav links and images on slider
image
  1. It would be better if the burger menu stretched to the full width and height of the browser window, and you also need to add a button to close this menu.
image

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 good.
A few points which you improve during prepare project for porfolio.

  • add transition for all elements with hover effect
  • for apply button hover state better to keep border in color as text.
    Screenshot 2024-12-13 at 14 15 33
  • mobile menu better to make on full width and height (see prev comments). it can be as menu in footer for mobile.

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