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

Phone catalog #565

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

Phone catalog #565

wants to merge 32 commits into from

Conversation

olshum8
Copy link

@olshum8 olshum8 commented Jan 18, 2025

I've been working on this site for a long time, and I'd really appreciate your feedback, suggestions, or comments. I'm currently in the process of redesigning components because I've found better ways of doing things. I'm not quite sure what the final result should look like, though. Thanks in advance :)

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.

sure, but don't forget to provide the demo link

@olshum8
Copy link
Author

olshum8 commented Jan 19, 2025

@olshum8 olshum8 requested a review from etojeDenys January 19, 2025 09:47
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.

Good job.
Pay attention on such moments.

  • somethin went wrong with favorite icons
    Screenshot 2025-01-19 at 16 29 01
  • add cursor pointer and hover effect for all interactive elements (links, buttons...). For example navigation buttons for main slider.
  • somethin went wrong with product details page
    Screenshot 2025-01-19 at 16 29 42
  • not visible border right in last visible slide
    Screenshot 2025-01-19 at 16 30 11
  • check path to category images and logo in footer
    Screenshot 2025-01-19 at 16 30 40
    Screenshot 2025-01-19 at 16 31 32

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.

Good job 👍
To improve:

  1. Add the topical title and favicon on the page
image
  1. When we change the number of products in the cart, the number needs to be updated in the upper right corner next to the icon.
image image
  1. Add the your github profile for all these links and open them in the new tab
image
  1. When we go to the product page, the page should automatically scroll up.
image
  1. Also, check the font family as on the design
image

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.

Hi, to improve:

  1. This comment still not fixed from the previous review. If you need help or any questions feel free in the fe_chat
image
  1. Where is the delete button for remove item from basket?
image
  1. Use a better favicon for the page
image

@olshum8
Copy link
Author

olshum8 commented Jan 28, 2025

Hi! I fixed the fonts, but now it looks a little terrible, but the specs are the same as on the figma design. Other points looks better now :)

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.

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.

6 participants