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

[Backoffice] Update in page pagination UIs vs functionalities #1269

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

HnKnA
Copy link
Contributor

@HnKnA HnKnA commented Nov 13, 2024

  • Enhance pagination UI in Location page
image

Use 'Enter' click to submit the page change

Pagination_Update.online-video-cutter.com.1.mp4

@nashtech-diepnguyenngoc1 nashtech-diepnguyenngoc1 marked this pull request as draft November 13, 2024 03:04
@HnKnA HnKnA marked this pull request as ready for review November 14, 2024 02:51
@HnKnA HnKnA marked this pull request as draft November 18, 2024 03:00
@HnKnA HnKnA marked this pull request as ready for review November 18, 2024 14:34
@mochacr0
Copy link
Contributor

mochacr0 commented Nov 21, 2024

Hi @HnKnA, I’ve been reading through your implementation, and I think it's coming along well. I have a suggestion regarding the pagination controls that you might want to consider:

Currently, we have states and their handlers (e.g., itemsPerPage with onItemsPerPageChange, and goToPage with onGoToPageChange/onGoToPageSubmit) that seem tightly coupled and are often used together. To ensure consistency and reduce the risk of mismatches, I suggest grouping related states and handlers into a single prop, such as a paginationControls object. This would make it clear that these values should always be passed together.

For example:

paginationControls?: {
  itemsPerPage: {
    value; //this is the current itemsPerPage state,
    onChange; //this is the current onItemsPerPageChange method,
  },
  goToPage: {
    value; //goToPage,
    onChange; //onGoToPageChange,
    onSubmit; //onGoToPageSubmit,
  }
}

Alternatively, if we need to keep itemsPerPage and goToPage separate, we could consider structuring them as distinct control props (itemsPerPageControl and goToPageControl). How do you think?

@HnKnA
Copy link
Contributor Author

HnKnA commented Nov 22, 2024

I prefer the approach of grouping related variables, and I will configure them today.

mochacr0
mochacr0 previously approved these changes Nov 25, 2024
Copy link
Contributor

@mochacr0 mochacr0 left a comment

Choose a reason for hiding this comment

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

Hi @HnKnA, I’ve noted a few points for potential refactoring that you can consider. When you have time, could you please take a look and let me know if they make sense? Thank you!

@mochacr0 mochacr0 dismissed their stale review November 25, 2024 02:51

Build failed

@@ -90,6 +90,29 @@ a {
background-color: black;
}

.pagination-helper {

Choose a reason for hiding this comment

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

We should move pagination css to Pagination.module.css.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've configured and added the Pagination.module.css file as you proposed.

image

Choose a reason for hiding this comment

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

Thanks for addressing the feedback.

Choose a reason for hiding this comment

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

Should create common/hooks and put this hook function to file usePaganation.ts.
Additionally, update the tsconfig.json to add a path alias for @commonHooks pointing to the common/hooks directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created and started using usePagination.ts instead of PaginationService.ts, and I've also updated tsconfig.json with the @commonHooks path alias.
image

Choose a reason for hiding this comment

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

Great, thanks. Please update optional param for usePagination and we can close this.

import { useState, useMemo, useCallback } from 'react';
import { DEFAULT_PAGE_NUMBER, DEFAULT_PAGE_SIZE } from '@constants/Common';

export const PAGE_SIZE_OPTION = [5, 10, 15, 20];

Choose a reason for hiding this comment

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

I think you can move this code to common const, beside with:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve updated the code to move PAGE_SIZE_OPTION to the common constants file

}

const usePagination = ({
initialPageNo = DEFAULT_PAGE_NUMBER,
Copy link

@vietvohoangnashtech vietvohoangnashtech Dec 16, 2024

Choose a reason for hiding this comment

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

Please make argument optional:
image
so that we can use this hook with default page value:
const { pageNo, totalPage, setTotalPage, changePage } = usePagination(); without

{
    initialPageNo: DEFAULT_PAGE_NUMBER,
    initialItemsPerPage: DEFAULT_PAGE_SIZE,
  }

argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The usePagination hook has been updated to make the arguments optional, with default values set for initialPageNo and initialItemsPerPage. Let me know if there’s anything else you’d like to adjust.

Copy link

vohoangvietuit

This comment was marked as off-topic.

Copy link

@vietvohoangnashtech vietvohoangnashtech left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the feedback! Approved.

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