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

add task solution #210

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

Conversation

romazh1988
Copy link

Copy link

@Viktor-Kost Viktor-Kost left a comment

Choose a reason for hiding this comment

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

GJ!

To improve:

  1. use svg for icons instead of png one

  2. too big. check sizes at figma

Screenshot 2024-03-11 at 12 37 00
  1. All interactive elements(links, buttons, pictures, icons) should have a hover effect and cursor pointer. User must intuitively understand that he can interact with them.
    Add hover effect to images and links

  2. remove default input outline and autocomplete styles

Screenshot 2024-03-11 at 12 39 35 Screenshot 2024-03-11 at 12 40 11
  1. add a correct type for email field to have a basic validation

  2. This button should lead to an appropriate section

Screenshot 2024-03-11 at 12 42 39
  1. Set a corect order for elements.
    Figma:
Screenshot 2024-03-11 at 12 43 26

Yours:
Screenshot 2024-03-11 at 12 43 58

  1. Decrease left indent
Screenshot 2024-03-11 at 12 44 22
  1. Check font styles
Screenshot 2024-03-11 at 12 45 17
  1. Titles like this aren't an image. You should use a correct font
Screenshot 2024-03-11 at 12 46 01
  1. Add a yellow background
Screenshot 2024-03-11 at 12 46 50
  1. Set correct elements order and decrease left indent
Screenshot 2024-03-11 at 12 47 42
  1. Shouldn't be centered
Screenshot 2024-03-11 at 12 48 21 Screenshot 2024-03-11 at 12 48 47 Screenshot 2024-03-11 at 12 49 03 Screenshot 2024-03-11 at 12 49 40
  1. Set a current year
Screenshot 2024-03-11 at 12 50 02
  1. Check an indent between elements
Screenshot 2024-03-11 at 12 51 00 Screenshot 2024-03-11 at 12 51 48
  1. Add this element
Screenshot 2024-03-11 at 12 52 08
  1. Check hover effect for this section. Since when I hover to this button I see the text from the righter button
Screenshot 2024-03-11 at 12 53 04
  1. check font styles and indents between elements
Screenshot 2024-03-11 at 12 54 40
  1. Not like at figma. Order, indents
Screenshot 2024-03-11 at 12 55 14

@romazh1988 romazh1988 requested a review from Viktor-Kost March 16, 2024 15:37
Copy link

@vadiimvooo vadiimvooo 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, a few changes required:

  1. Fix padding for the sections, check for the appropriate one in mockup
    image
  2. This part should be fixed as well, please check the mockup
    image
  3. Scaled element should not overlap the neighbours.
    image
  4. This block should be fixed as well, please check the mockup
    image
  5. Please check the mockup, the distance between block is wrong
    image
    6 This block is also have wrong distances and elements with wrong sizes, please fix it as well
    image
  6. Address should be clickable
    image
  7. The same probles exists on other sizes of screen, so please fix it.

@romazh1988 romazh1988 requested a review from vadiimvooo March 18, 2024 11:53
Copy link

@l4st1m0za l4st1m0za left a comment

Choose a reason for hiding this comment

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

  1. Add hover effect on all interactive elements
image
  1. This button does not work
image
  1. Text from section abbover covers the elements under it
image
  1. In element like this, all element should be clickable, not only arrow, cross, etc.
image

@romazh1988 romazh1988 requested a review from l4st1m0za March 18, 2024 14:09
Copy link

@l4st1m0za l4st1m0za left a comment

Choose a reason for hiding this comment

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

  1. This button should show text, in the left side from it, not to overlap next sectoin
image
  1. Only the link, should have the hover effect, not the label
image
  1. This element should hover at once, not separetely
image
  1. These images still overlap neighbour elements
image

@romazh1988 romazh1988 requested a review from l4st1m0za March 19, 2024 08:39
Copy link

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

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