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

commit -for fe-chat problem with swiper #196

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

Conversation

MykolaButylkov
Copy link

@MykolaButylkov MykolaButylkov commented Jul 26, 2023

Copy link
Contributor

@mykhalenych mykhalenych left a comment

Choose a reason for hiding this comment

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

Nice start, but delete all comments before commit, waiting for next functionality

src/pages/components/ProductCard.tsx Outdated Show resolved Hide resolved
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.

Add some message if there are no item for category
2023-08-01_11h50_07

Check this case. You should reset selected page on itemsPerPage change
https://github.com/mate-academy/react_phone-catalog/assets/61904995/24e043ba-7c53-447e-8a11-580f902126a6

Add to card button should have cursor:pointer
https://github.com/mate-academy/react_phone-catalog/assets/61904995/2d60b018-f3f6-422f-becc-2d5de8d5e562

Those buttons didn't work
2023-08-01_11h53_54

Add message if there no results on search
2023-08-01_11h55_16

Check this case
2023-08-01_11h55_58
2023-08-01_11h57_15

src/pages/AccessoriesPage.tsx Outdated Show resolved Hide resolved
src/pages/AccessoriesPage.tsx Outdated Show resolved Hide resolved
src/pages/AccessoriesPage.tsx Outdated Show resolved Hide resolved
src/pages/AccessoriesPage.tsx Outdated Show resolved Hide resolved
src/pages/CardPage.tsx Outdated Show resolved Hide resolved
src/pages/components/ProductCard.tsx Outdated Show resolved Hide resolved
src/pages/components/ProductCard.tsx Outdated Show resolved Hide resolved
src/pages/components/ProductCard.tsx Outdated Show resolved Hide resolved
src/pages/components/ProductsSlider.tsx Outdated Show resolved Hide resolved
src/pages/components/SearchBar.tsx Outdated Show resolved Hide resolved
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.

  • center horizontally page content
    Screenshot from 2023-08-05 15-28-17

  • Footer content should be limited to the same width as the page content
    Screenshot from 2023-08-05 15-35-43
    Screenshot from 2023-08-05 15-28-27

  • make responsive footer and header
    Screenshot from 2023-08-05 15-31-48
    Screenshot from 2023-08-05 15-32-02

  • check this bug on tablets

chrome-capture-2023-7-5

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.

Hello!
Nice work!
Pay attention at these previous mentor's comments
#196 (comment)
#196 (review) (page should be centered)
Screenshot 2023-08-07 at 21 36 43

To improve:

  1. make colors be selectable similar to capacity one
Screen.Recording.2023-08-07.at.21.39.46.mov
  1. Remove extra black background at images and use correct sizes since the last image is distorted
Screen.Recording.2023-08-07.at.21.41.06.mov
  1. add a condition to avoid selecting less than 1
Screenshot 2023-08-07 at 21 43 27
  1. case: I enter a query. Found nothing. Click at logo. Click at tab. See no products and no query at input. It's a little bit confusing. Either leave an input query, or reload page
Screen.Recording.2023-08-07.at.21.45.58.mov
  1. As a recommendation: if a product is selected write "remove from card" at the button
Screenshot 2023-08-07 at 21 49 42

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.

Well done, Mykola! 😉

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