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

Create Phone catalog #519

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

Create Phone catalog #519

wants to merge 37 commits into from

Conversation

Anothar
Copy link

@Anothar Anothar commented Oct 14, 2024

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work!
Screenshot 2024-10-14 at 16 03 02
The page should be set to the first page if the search was implemented
Screenshot 2024-10-14 at 16 01 44
Check the layout here
Screenshot 2024-10-14 at 16 00 35
Check the number of products
Screenshot 2024-10-14 at 16 00 11
The button doesn't work

@Anothar
Copy link
Author

Anothar commented Oct 14, 2024

image

I don't understand how you found this error, I can't seem to reproduce the same situation as you.

@Anothar
Copy link
Author

Anothar commented Oct 14, 2024

Mine
image

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work!
Consider redirecting to the first page when the sorting or per page quantity or search query change occurs
Screenshot 2024-10-14 at 17 14 40
Consider adding a link to your GH here

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

The main gallery seems a bit broken only in Mozilla so you can skip this comment

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 a favicon on the page
image
  1. Add hover effects for all clickable elements on the page
image
  1. Links in the header should span the entire height of the header
image
  1. These buttons must be are on the same line
image
  1. Open these additional links in the new tab
image
  1. Need to remove scroll when the menu is open
image
  1. Also, add a transition when the menu disappears on the page
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.

Almost done!
To improve:

  1. This comment still not fixed from the previous review
image image

These buttons must be are on the same vertical lines
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.

Great! 🔥

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.

3 participants