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

Solution #571

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

Solution #571

wants to merge 4 commits into from

Conversation

Bajkowsky
Copy link

Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

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

Hey, great job on the implementation! There are still a few things to improve. I’ve reviewed the app in desktop view, and while it looks okay overall, the user experience with the loaders feels off. They appear too frequently—almost every time I click on something, there’s a loader.

The transitions also feel a bit overdone; maybe stick to a simple opacity change instead. On the home page, the carousel behaves strangely. When I click, it sometimes swipes one item at a time, but other times it swipes all four items at once. For the banner images, the transitions feel awkward—just a simple swipe effect would work better.Even the buttons have transitions, which makes them unclickable for a short while. This creates a frustrating experience for the user.

On mobile this spacing between cards looks ugly
{836DA468-3FF9-4E8D-AB19-03EA01844A2B}

I have noticed some dead code for example redux slices for comments, posts, users that aren't used.

@Bajkowsky Bajkowsky requested a review from Zibi95 January 27, 2025 13:44
Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

  • you don't need to reload the entire page (phones, tablets, accessories) when changing the sorting or items per page - just rerender the list of products.
  • text "Items on page" is slightly displaced
    image
  • on phones, tablets and accesories pages, elements remain fixed while margins expand as the page resizes. they should adjust dynamically, as the current behavior results in disproportionately large margins
    image

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

  • layout isn't responsive for pages: phones, tablets and accessories, it's fixed with breakpoints. here you have differences between fixed and responsive layout. and here you have some example how looks responsive layout with breakpoints
  • you don't need to reload the entire page (phones, tablets, accessories) when changing the sorting or items per page - just rerender the list of products

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.

4 participants