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

message #183

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

message #183

wants to merge 11 commits into from

Conversation

Ilia991
Copy link

@Ilia991 Ilia991 commented Jun 22, 2023

Copy link

@polosanya polosanya left a comment

Choose a reason for hiding this comment

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

Please don't forget to add a demo link to the PR description, so we can properly check your work 🙂
image
Also, make sure to fix all eslint problems
image

@Ilia991 Ilia991 requested a review from polosanya August 5, 2023 16:40
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.

Great job.
Needs some improvements.

  • can't to close menu when click on button.
    chrome-capture-2023-7-6
  • footer should be in the bottom of the page
    Screenshot from 2023-08-06 18-03-09
  • check styles in cart page.
    space between value and button.
    Screenshot from 2023-08-06 17-56-22
    content should have same width as footer
    Screenshot from 2023-08-06 17-56-31

Copy link

@polosanya polosanya left a comment

Choose a reason for hiding this comment

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

Keep up the good work!
Please check if you fixed all eslint problems
image

  1. Make sure that the transition is added for the default state of the element, not hover, so it works smoothly both ways
    smooth-transition
  2. Why is the page scrolled to the top if the user likes some phone?
    image
  3. Typo in "mobile"
    image

@Ilia991 Ilia991 requested a review from polosanya August 8, 2023 08:53
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! Lets improve it

  1. add cursor pointer to selects(for all interactive elements actually)
    image

  2. did you implement product-detail-page?

  3. don't add more phones on click when selected
    image
    you can do it here if heeded
    image

  4. select should hide on second click
    image

request review when you finish with all pages

@Ilia991 Ilia991 requested a review from sTorba24 October 2, 2023 11:53
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.

You should clear search qeury on tab change, cuz it stop works
https://github.com/mate-academy/react_phone-catalog/assets/61904995/6bd5aeeb-d810-443d-a656-4953a3c7e52b

add favicon

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.

Good work, but lets make some improvement

  1. please make some shorter delay on debounce while search
  2. increase area for select product
  3. also add padding and cursor pointer for better clickable
    image

@Ilia991 Ilia991 requested a review from maxim2310 October 5, 2023 11:14
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.

Fix issues below before adding project into porfolio:
Pay attention input should works on full self width
https://github.com/mate-academy/react_phone-catalog/assets/61904995/6559fc74-a70b-463c-bbfe-7d0efb09b73c

Broken links
2023-10-05_15h21_46

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.

6 participants