Feature/os 50 fe create pagination category page mob #74
Conversation
….com/Vadim-Kostetskyi/Online-store into feature/OS-50-FE-Create-pagination-Category-Page-mob
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
olenamshrv
requested review from
AlexKutkanych,
denred,
Vadim-Kostetskyi and
tetiananosenko
| <> | ||
| <Header /> | ||
| <div className={styles.main}>{children}</div> | ||
| <div className={`${styles.main} ${isLoading ? styles.mainMaxHeight : ''}`}> |
There was a problem hiding this comment.
| <div className={`${styles.main} ${isLoading ? styles.mainMaxHeight : ''}`}> | |
| <div className={getValidClassNames(styles.main, { [styles.mainMaxHeight]: isLoading })}> | |
| className={`${styles.paginationButton} ${ | ||
| isDisabled ? styles.paginationDisabled : '' | ||
| }`} |
There was a problem hiding this comment.
consider using getValidClassNames
| {isDisabled ? ( | ||
| isPrevious ? ( | ||
| <ArrowLeftGrey /> | ||
| ) : ( | ||
| <ArrowRightGrey /> | ||
| ) | ||
| ) : isPrevious ? ( | ||
| <ArrowLeft /> | ||
| ) : ( | ||
| <ArrowRight /> | ||
| )} |
There was a problem hiding this comment.
consider using a helper for readability
There was a problem hiding this comment.
at least create a separate constant in this component and don't use nested ternary operator
| isDisabled, | ||
| handleClick, | ||
| }) => { | ||
| return ( |
| className={`${styles.pageButton} | ||
| ${ | ||
| pageNumber === activePage | ||
| ? styles.pageButtonActive | ||
| : '' | ||
| }`} |
| className={`${styles.pageButton} | ||
| ${ | ||
| pageNumber === activePage | ||
| ? styles.pageButtonActive | ||
| : '' | ||
| }`} |
| className={`${styles.pageButton} | ||
| ${ | ||
| pagesAmount === activePage | ||
| ? styles.pageButtonActive | ||
| : '' | ||
| }`} |
There was a problem hiding this comment.
the same
too much repetitive code, consider using a function
src/pages/ProductsGridPage/index.tsx
Outdated
| useEffect(() => { | ||
| searchProducts({ | ||
| page: FIRST_PAGE, | ||
| size: PRODUCT_GRID_SIZE, | ||
| page: Number(PageNumbers.FIRST_PAGE), |
There was a problem hiding this comment.
it already seems like a number
src/pages/ProductsGridPage/index.tsx
Outdated
| const handleClick = (body: BodySearchProducts) => { | ||
| searchProducts({ | ||
| page: FIRST_PAGE, | ||
| size: PRODUCT_GRID_SIZE, | ||
| page: Number(PageNumbers.FIRST_PAGE), | ||
| size: gridPageSize, | ||
| body, | ||
| }); | ||
| }; |
src/pages/ProductsGridPage/index.tsx
Outdated
| const handlePagination = (pageNumber: PageNumbers) => { | ||
| searchProducts({ | ||
| page: Number(pageNumber), | ||
| size: gridPageSize, | ||
| body: | ||
| activeButton === Category.CLOTHING || isMobile | ||
| ? { | ||
| category: activeButton as Category, | ||
| } | ||
| : { | ||
| subcategory: activeButton as Subcategory, | ||
| }, | ||
| }); | ||
| }; |
src/assets/svgs/ArrowLeftGrey.tsx
Outdated
| @@ -0,0 +1,23 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const ArrowLeftGrey = (): JSX.Element => { | |||
There was a problem hiding this comment.
is it the same icon as ArrowLeft?
please use ArrowLeft and change color
src/assets/svgs/ArrowRight.tsx
Outdated
| @@ -0,0 +1,23 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const ArrowRight = () => { | |||
src/assets/svgs/ArrowRightGrey.tsx
Outdated
| @@ -0,0 +1,23 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const ArrowRightGrey = () => { | |||
| @@ -31,7 +31,6 @@ const CoreSwiper: FC<CoreSwiperProps> = ({ | |||
| effect="fade" | |||
| fadeEffect={{ crossFade: true }} | |||
| rewind | |||
| loop | |||
There was a problem hiding this comment.
In case of using "loop" the following error occurs while resizing the window:
It is mentioned here:
"Uncaught error when unmounting Swiper with loop on window resize #7265"
nolimits4web/swiper#7265
Also it seems the functionality has not been impacted by removing the "loop".
|
|
||
| import styles from './index.module.scss'; | ||
|
|
||
| const Ellipsis = (): React.JSX.Element => ( |
| setActivePage(pageNumber); | ||
| }, []); | ||
|
|
||
| const handleButtonClick = useCallback((increment: number) => { |
There was a problem hiding this comment.
please use currying
| <ul className={styles.pager}> | ||
| <li className={styles.pageText}>{t('page')}</li> | ||
| {Number(pagesAmount) <= DEFAULT_PAGES_AMOUNT ? ( | ||
| pagesList.map(pageNumber => { |
There was a problem hiding this comment.
move to a separate constant and wrap into useMemo
| {Number(pagesAmount) - 3} | ||
| </li> | ||
| )} | ||
| {Number(activePage) > Number(pagesAmount) - 3 && |
There was a problem hiding this comment.
sorry, but this component is unreadable
we have to rewrite it
There was a problem hiding this comment.
why do you need so many separate cases?
There was a problem hiding this comment.
Due to design there are the following cases:
and additionally two more (when page 3 or the 3rd page back from the last page is active:
src/pages/ProductsGridPage/index.tsx
Outdated
| const { t } = useTranslation(); | ||
|
|
||
| const [activeButton, setActiveButton] = useState<string>( | ||
| getButtons(t)[0].value, |
There was a problem hiding this comment.
add optional chaining
src/utils/constants.ts
Outdated
| @@ -6,7 +6,33 @@ export const MenuItem = { | |||
| KIDS: 'Kids', | |||
| } as const; | |||
|
|
|||
| export const FIRST_PAGE = 1; | |||
| export enum PageNumbers { | |||
There was a problem hiding this comment.
doesn't seem right to me
No description provided.