feat: add pagination feature in blog page #3595
Conversation
priyanshuxkumar
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces a comprehensive pagination system for the blog page. The implementation includes creating helper functions like Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hello, @coderabbitai[bot]! 👋🏼 |
|
@asyncapi-bot i'll start reviewing the changes for the pagination feature implementation. 🔗 analysis chain 🏁 scripts executed the following scripts were executed for the analysis: script: length of output: 9907 ✅ Actions performedReview triggered.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3595 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 732 732
=========================================
Hits 732 732 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3595--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
components/helpers/applyFilter.ts (1)
140-142: Consider filtering out pagination params before processing.While the current implementation works correctly by skipping the 'page' parameter, consider a more maintainable approach by filtering out pagination-related parameters before processing.
Here's a suggested improvement:
export const onFilterApply = ( inputData: DataObject[], onFilter: (result: DataObject[], query: Filter) => void, query: Filter ): void => { let result = inputData; if (query && Object.keys(query).length >= 1) { - Object.keys(query).forEach((property) => { - if (property === 'page') { - return; - } + // Filter out pagination params + const filterParams = Object.keys(query).filter(key => key !== 'page'); + filterParams.forEach((property) => { const res = result.filter((e) => { // ... rest of the filtering logic }); result = res; }); } onFilter(result, query); };This approach:
- Makes the intention clearer by explicitly filtering out pagination params
- Avoids using early returns within forEach
- Is more extensible if more pagination-related params need to be excluded in the future
components/pagination/PaginationItem.tsx (1)
4-5: Remove unnecessary eslint-disable comment.The empty eslint-disable comment can be removed as it serves no purpose.
- // eslint-disable-next-line prettier/prettiercomponents/helpers/usePagination.ts (1)
14-30: Add input validation for edge cases.While the implementation is solid, consider adding validation for:
- Negative or zero itemsPerPage
- Empty items array
export function usePagination<T>(items: T[], itemsPerPage: number) { + if (itemsPerPage <= 0) { + throw new Error('itemsPerPage must be greater than 0'); + } + const [currentPage, setCurrentPage] = useState(1); - const maxPage = Math.ceil(items.length / itemsPerPage); + const maxPage = items.length ? Math.ceil(items.length / itemsPerPage) : 1;components/pagination/Pagination.tsx (2)
6-7: Remove unnecessary eslint-disable comment.The empty eslint-disable comment can be removed as it serves no purpose.
- // eslint-disable-next-line prettier/prettier
55-75: Enhance accessibility and reduce CSS duplication.
- Add aria-labels to SVG elements for better accessibility
- Extract duplicate button styles to a common class
+const commonButtonStyles = ` + font-normal flex h-[34px] items-center justify-center gap-2 rounded bg-white px-4 + py-[7px] text-sm leading-[17px] tracking-[-0.01em] +`; {/* Previous button */} <button onClick={() => handlePageChange(currentPage - 1)} disabled={currentPage === 1} - className={` - font-normal flex h-[34px] items-center justify-center gap-2 rounded bg-white px-4 - py-[7px] text-sm leading-[17px] tracking-[-0.01em] + className={`${commonButtonStyles} ${currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50'} `} + aria-label="Previous page" > <svg width='20' height='20' viewBox='0 0 24 24' fill='none' xmlns='http://www.w3.org/2000/svg' className='stroke-current' + aria-hidden="true" > {/* Next button */} <button onClick={() => handlePageChange(currentPage + 1)} disabled={currentPage === totalPages} - className={` - font-normal flex h-[34px] items-center justify-center gap-2 rounded bg-white px-4 - py-[7px] text-sm leading-[17px] tracking-[-0.01em] + className={`${commonButtonStyles} ${currentPage === totalPages ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50'} `} + aria-label="Next page" >Also applies to: 99-119
pages/blog/index.tsx (1)
43-56: Add scroll to top behavior on page change.When navigating between pages, consider scrolling to the top for better user experience.
const handlePageChange = (page: number) => { setCurrentPage(page); const currentFilters = { ...router.query, page: page.toString() }; router.push( { pathname: router.pathname, query: currentFilters }, undefined, { shallow: true } - ); + ).then(() => { + window.scrollTo({ top: 0, behavior: 'smooth' }); + }); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/helpers/applyFilter.ts(1 hunks)components/helpers/usePagination.ts(1 hunks)components/pagination/Pagination.tsx(1 hunks)components/pagination/PaginationItem.tsx(1 hunks)pages/blog/index.tsx(3 hunks)
🔇 Additional comments (3)
components/pagination/PaginationItem.tsx (1)
19-31: Great implementation with accessibility in mind!The component is well-structured with:
- Clear prop types and documentation
- Proper accessibility attributes (aria-current)
- Semantic button element with appropriate styling
components/pagination/Pagination.tsx (1)
21-122: Well-implemented pagination logic!The component demonstrates:
- Smart handling of different page number scenarios
- Clean separation of concerns
- Proper validation in handlePageChange
pages/blog/index.tsx (1)
154-168: Well-integrated pagination system!The pagination integration demonstrates:
- Clean integration with the existing blog page
- Proper state management
- Good user experience with loading states
pages/blog/index.tsx
Outdated
| useEffect(() => { | ||
| const pageFromQuery = parseInt(router.query.page as string, 10); | ||
|
|
||
| if (!Number.isNaN(pageFromQuery) && pageFromQuery >= 1 && pageFromQuery !== currentPage) { | ||
| setCurrentPage(pageFromQuery); | ||
| } | ||
| }, [router.query.page]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid page numbers.
When an invalid page number is provided in the URL, the code should handle it gracefully.
useEffect(() => {
const pageFromQuery = parseInt(router.query.page as string, 10);
- if (!Number.isNaN(pageFromQuery) && pageFromQuery >= 1 && pageFromQuery !== currentPage) {
+ if (!Number.isNaN(pageFromQuery)) {
+ if (pageFromQuery >= 1 && pageFromQuery <= maxPage && pageFromQuery !== currentPage) {
setCurrentPage(pageFromQuery);
+ } else {
+ // Reset to page 1 for invalid page numbers
+ handlePageChange(1);
+ }
}
- }, [router.query.page]);
+ }, [router.query.page, maxPage]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Thank you for the suggestion to add error handling for invalid page numbers. I implemented the proposed code, but I am encountering an issue where navigating to any page (e.g., ?page=2) redirects me back to ?page=1.
It seems like the handlePageChange(1) function is being triggered unnecessarily, even when the pageFromQuery is valid.
There was a problem hiding this comment.
@priyanshuxkumar I understand the issue. The problem occurs because we're validating against maxPage before it's properly initialized. Let's modify the code to ensure we only validate when we have a valid maxPage:
useEffect(() => {
const pageFromQuery = parseInt(router.query.page as string, 10);
if (!Number.isNaN(pageFromQuery) && maxPage > 0) {
if (pageFromQuery >= 1 && pageFromQuery <= maxPage && pageFromQuery !== currentPage) {
setCurrentPage(pageFromQuery);
} else if (pageFromQuery < 1 || pageFromQuery > maxPage) {
// Only reset to page 1 if the page number is actually invalid
handlePageChange(1);
}
}
}, [router.query.page, maxPage, currentPage]);This should fix the issue by:
- Only performing validation when
maxPageis properly initialized (maxPage > 0) - Only resetting to page 1 when the page number is actually out of bounds
Let me know if this resolves the issue!
There was a problem hiding this comment.
@priyanshuxkumar Great to hear that the pagination is working correctly now! 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pages/blog/index.tsx (3)
40-41: Consider moving postsPerPage to configuration.The hardcoded value of 9 posts per page would be better managed as a configuration constant, making it easier to adjust across different viewports or requirements.
+// Add to src/config/blog.ts or similar +export const POSTS_PER_PAGE = 9; + // In BlogIndexPage - const postsPerPage = 9; + const postsPerPage = POSTS_PER_PAGE;
164-168: Improve loader condition logic.The current loader condition might show unnecessarily when
currentItemsis empty butpostsexist. Consider checking both arrays.- {Object.keys(currentItems).length > 0 && !isClient && ( + {Object.keys(posts).length > 0 && !isClient && (
169-173: Enhance pagination container accessibility.Add appropriate ARIA attributes to improve accessibility for screen readers.
- <div className='mt-8 w-full'> + <div className='mt-8 w-full' role="navigation" aria-label="Blog posts pagination">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/blog/index.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (2)
pages/blog/index.tsx (2)
4-4: LGTM! Import statements are well-organized.The new imports for pagination components follow the existing import structure and use proper path aliases.
Also applies to: 10-10
43-69: Well-implemented page change handling and URL synchronization!The code effectively:
- Updates page state
- Synchronizes URL query parameters
- Handles invalid page numbers gracefully
- Uses shallow routing for better performance
components/helpers/applyFilter.ts
Outdated
| if (property === 'page') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can't we handle filters in a better approach?
Think in an approach where we look for some specific variables from query params here, not all.
There was a problem hiding this comment.
` const nonFilterableKeys = ['page'];
if (query && Object.keys(query).length >= 1) {
Object.keys(query).forEach((property) => {
if (nonFilterableKeys.includes(property)) {
return; // Skip non-filterable keys like 'page'
}
`
Dynamically handles filters while explicitly excluding non-filterable keys like 'page'
| * @returns {T[]} currentItems - Items for the current page | ||
| * @returns {number} maxPage - Total number of pages | ||
| */ | ||
| export function usePagination<T>(items: T[], itemsPerPage: number) { |
There was a problem hiding this comment.
It takes a prop of items(all items) and returns the only item that has to be displayed on the current page
components/pagination/Pagination.tsx
Outdated
| const getPageNumbers = () => { | ||
| const pages = []; | ||
|
|
||
| if (totalPages <= 7) { | ||
| for (let i = 1; i <= totalPages; i++) pages.push(i); | ||
| } else { | ||
| pages.push(1); | ||
| if (currentPage > 3) { | ||
| pages.push('ellipsis1'); | ||
| } | ||
|
|
||
| for (let i = Math.max(2, currentPage - 1); i <= Math.min(totalPages - 1, currentPage + 1); i++) { | ||
| pages.push(i); | ||
| } | ||
|
|
||
| if (currentPage < totalPages - 2) { | ||
| pages.push('ellipsis2'); | ||
| } | ||
|
|
||
| pages.push(totalPages); | ||
| } | ||
|
|
||
| return pages; | ||
| }; |
There was a problem hiding this comment.
Can you please explain the else part of this logic? It seems to be very weird.
There was a problem hiding this comment.
i have updated this . basically it return the number of pages for display on pagination -
components/pagination/Pagination.tsx
Outdated
| <button | ||
| onClick={() => handlePageChange(currentPage - 1)} | ||
| disabled={currentPage === 1} | ||
| className={` | ||
| font-normal flex h-[34px] items-center justify-center gap-2 rounded bg-white px-4 | ||
| py-[7px] text-sm leading-[17px] tracking-[-0.01em] | ||
| ${currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50'} | ||
| `} | ||
| > |
There was a problem hiding this comment.
We have existing Button element, can't we use that?
components/pagination/Pagination.tsx
Outdated
| <svg | ||
| width='20' | ||
| height='20' | ||
| viewBox='0 0 24 24' | ||
| fill='none' | ||
| xmlns='http://www.w3.org/2000/svg' | ||
| className='stroke-current' | ||
| > | ||
| <path d='M15 18L9 12L15 6' strokeWidth='2' strokeLinecap='round' strokeLinejoin='round' /> | ||
| </svg> |
There was a problem hiding this comment.
Add this SVG image as part of icons. Don't add the SVG code directly in the components.
…ynamically and update the function to render page number on pagination component
…website into feat-pagination
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
components/icons/Previous.tsx (1)
9-18: Enhance SVG accessibilityAdd appropriate ARIA attributes and descriptive elements for screen readers.
<svg width='20' height='20' viewBox='0 0 24 24' fill='none' xmlns='http://www.w3.org/2000/svg' className='stroke-current' + role="img" + aria-label="Previous page" > + <title>Previous page</title> <path d='M15 18L9 12L15 6' strokeWidth='2' strokeLinecap='round' strokeLinejoin='round' /> </svg>components/pagination/Pagination.tsx (3)
11-12: Remove unnecessary empty eslint-disable commentThis empty prettier/eslint disable comment can be removed as it serves no purpose.
26-31: Add input validation for propsAdd validation to ensure
totalPagesis positive andcurrentPageis within valid range.export default function Pagination({ totalPages, currentPage, onPageChange }: PaginationProps) { + if (totalPages < 1) { + throw new Error('totalPages must be positive'); + } + if (currentPage < 1 || currentPage > totalPages) { + throw new Error('currentPage must be between 1 and totalPages'); + } + const handlePageChange = (page: number) => {
36-61: Extract magic numbers into named constantsThe pagination logic uses magic numbers (7, 3, 2) which should be extracted into named constants for better maintainability.
+const MAX_VISIBLE_PAGES = 7; +const BOUNDARY_PAGES = 3; +const SIBLING_PAGES = 1; + const getPageNumbers = (): (number | string)[] => { - if (totalPages <= 7) { + if (totalPages <= MAX_VISIBLE_PAGES) { return Array.from({ length: totalPages }, (_, i) => i + 1); } const pages: (number | string)[] = [1]; - if (currentPage > 3) { + if (currentPage > BOUNDARY_PAGES) { pages.push('ellipsis1'); } - const start = Math.max(2, currentPage - 1); - const end = Math.min(totalPages - 1, currentPage + 1); + const start = Math.max(2, currentPage - SIBLING_PAGES); + const end = Math.min(totalPages - 1, currentPage + SIBLING_PAGES);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/helpers/applyFilter.ts(1 hunks)components/icons/Next.tsx(1 hunks)components/icons/Previous.tsx(1 hunks)components/pagination/Pagination.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/icons/Next.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/helpers/applyFilter.ts
components/icons/Previous.tsx
Outdated
| /** | ||
| * @description Icons for Previous button in pagination | ||
| */ | ||
| export default function IconPrevios() { |
There was a problem hiding this comment.
Fix typo in component name: IconPrevios → IconPrevious
The component name has a typo that should be corrected for clarity and consistency.
-export default function IconPrevios() {
+export default function IconPrevious() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function IconPrevios() { | |
| export default function IconPrevious() { |
components/pagination/Pagination.tsx
Outdated
| currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | ||
| }`} | ||
| text='Previous' | ||
| icon={<IconPrevios />} |
There was a problem hiding this comment.
Update import to use corrected icon name
Once the IconPrevios component is renamed to IconPrevious, update this import reference.
-icon={<IconPrevios />}
+icon={<IconPrevious />}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| icon={<IconPrevios />} | |
| icon={<IconPrevious />} |
components/pagination/Pagination.tsx
Outdated
| return ( | ||
| <div className='font-inter flex items-center justify-center gap-8'> | ||
| {/* Previous button */} | ||
| <Button | ||
| onClick={() => handlePageChange(currentPage - 1)} | ||
| className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ | ||
| currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | ||
| }`} | ||
| text='Previous' | ||
| icon={<IconPrevios />} | ||
| iconPosition={ButtonIconPosition.LEFT} | ||
| /> | ||
|
|
||
| {/* Page numbers */} | ||
| <div className='flex gap-2'> | ||
| {getPageNumbers().map((page) => | ||
| typeof page === 'number' ? ( | ||
| <PaginationItem | ||
| key={page} | ||
| pageNumber={page} | ||
| isActive={page === currentPage} | ||
| onPageChange={handlePageChange} | ||
| /> | ||
| ) : ( | ||
| <span | ||
| key={page} | ||
| className='font-inter flex size-10 items-center justify-center text-sm font-semibold text-[#6B6B6B]' | ||
| > | ||
| ... | ||
| </span> | ||
| ) | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Next button */} | ||
| <Button | ||
| onClick={() => handlePageChange(currentPage + 1)} | ||
| className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ | ||
| currentPage === totalPages ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | ||
| }`} | ||
| text='Next' | ||
| icon={<IconNext />} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and extract styles
The pagination component needs accessibility improvements and style cleanup:
- Add ARIA labels for navigation
- Extract complex conditional styles into utility classes or CSS modules
- Add keyboard navigation support
return (
- <div className='font-inter flex items-center justify-center gap-8'>
+ <nav
+ role="navigation"
+ aria-label="Pagination"
+ className='font-inter flex items-center justify-center gap-8'
+ >
{/* Previous button */}
<Button
onClick={() => handlePageChange(currentPage - 1)}
+ disabled={currentPage === 1}
className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${
currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50'
}`}
text='Previous'
icon={<IconPrevios />}
iconPosition={ButtonIconPosition.LEFT}
+ aria-label="Go to previous page"
/>
{/* Page numbers */}
- <div className='flex gap-2'>
+ <div className='flex gap-2' role="list">
{getPageNumbers().map((page) =>
typeof page === 'number' ? (
<PaginationItem
key={page}
pageNumber={page}
isActive={page === currentPage}
onPageChange={handlePageChange}
+ aria-label={`Go to page ${page}`}
+ aria-current={page === currentPage ? 'page' : undefined}
/>
) : (
<span
key={page}
className='font-inter flex size-10 items-center justify-center text-sm font-semibold text-[#6B6B6B]'
+ aria-hidden="true"
>
...
</span>
)
)}
</div>
{/* Next button */}
<Button
onClick={() => handlePageChange(currentPage + 1)}
+ disabled={currentPage === totalPages}
className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${
currentPage === totalPages ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50'
}`}
text='Next'
icon={<IconNext />}
+ aria-label="Go to next page"
/>
- </div>
+ </nav>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div className='font-inter flex items-center justify-center gap-8'> | |
| {/* Previous button */} | |
| <Button | |
| onClick={() => handlePageChange(currentPage - 1)} | |
| className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ | |
| currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | |
| }`} | |
| text='Previous' | |
| icon={<IconPrevios />} | |
| iconPosition={ButtonIconPosition.LEFT} | |
| /> | |
| {/* Page numbers */} | |
| <div className='flex gap-2'> | |
| {getPageNumbers().map((page) => | |
| typeof page === 'number' ? ( | |
| <PaginationItem | |
| key={page} | |
| pageNumber={page} | |
| isActive={page === currentPage} | |
| onPageChange={handlePageChange} | |
| /> | |
| ) : ( | |
| <span | |
| key={page} | |
| className='font-inter flex size-10 items-center justify-center text-sm font-semibold text-[#6B6B6B]' | |
| > | |
| ... | |
| </span> | |
| ) | |
| )} | |
| </div> | |
| {/* Next button */} | |
| <Button | |
| onClick={() => handlePageChange(currentPage + 1)} | |
| className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ | |
| currentPage === totalPages ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | |
| }`} | |
| text='Next' | |
| icon={<IconNext />} | |
| /> | |
| </div> | |
| ); | |
| return ( | |
| <nav | |
| role="navigation" | |
| aria-label="Pagination" | |
| className='font-inter flex items-center justify-center gap-8' | |
| > | |
| {/* Previous button */} | |
| <Button | |
| onClick={() => handlePageChange(currentPage - 1)} | |
| disabled={currentPage === 1} | |
| className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ | |
| currentPage === 1 ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | |
| }`} | |
| text='Previous' | |
| icon={<IconPrevios />} | |
| iconPosition={ButtonIconPosition.LEFT} | |
| aria-label="Go to previous page" | |
| /> | |
| {/* Page numbers */} | |
| <div className='flex gap-2' role="list"> | |
| {getPageNumbers().map((page) => | |
| typeof page === 'number' ? ( | |
| <PaginationItem | |
| key={page} | |
| pageNumber={page} | |
| isActive={page === currentPage} | |
| onPageChange={handlePageChange} | |
| aria-label={`Go to page ${page}`} | |
| aria-current={page === currentPage ? 'page' : undefined} | |
| /> | |
| ) : ( | |
| <span | |
| key={page} | |
| className='font-inter flex size-10 items-center justify-center text-sm font-semibold text-[#6B6B6B]' | |
| aria-hidden="true" | |
| > | |
| ... | |
| </span> | |
| ) | |
| )} | |
| </div> | |
| {/* Next button */} | |
| <Button | |
| onClick={() => handlePageChange(currentPage + 1)} | |
| disabled={currentPage === totalPages} | |
| className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ | |
| currentPage === totalPages ? 'cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' | |
| }`} | |
| text='Next' | |
| icon={<IconNext />} | |
| aria-label="Go to next page" | |
| /> | |
| </nav> | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/pagination/Pagination.tsx (5)
11-12: Remove unnecessary eslint-disable comment.The empty eslint-disable comment for prettier/prettier can be safely removed as it serves no purpose.
- // eslint-disable-next-line prettier/prettier -
23-25: Enhance component documentation.The current JSDoc could be more descriptive. Consider adding:
/** * This is the Pagination component. It displays a list of page numbers that can be clicked to navigate. + * + * @param {Object} props + * @param {number} props.totalPages - Total number of pages to display + * @param {number} props.currentPage - Currently active page number + * @param {Function} props.onPageChange - Callback function triggered when page changes + * @returns {JSX.Element} Pagination navigation component + * + * @example + * <Pagination + * totalPages={10} + * currentPage={1} + * onPageChange={(page) => setCurrentPage(page)} + * /> */
33-35: Improve helper function documentation.The current JSDoc should better describe the function's behavior and return value format.
/** - * @returns number of pages shows in Pagination. + * Generates an array of page numbers and ellipsis markers for pagination display. + * For <= 7 pages, returns all page numbers. + * For > 7 pages, returns a subset with ellipsis markers. + * + * @returns {(number|string)[]} Array containing page numbers and 'ellipsis1'/'ellipsis2' strings + * @example + * // Returns [1, 2, 3, 4, 5] for totalPages=5 + * // Returns [1, 'ellipsis1', 4, 5, 6, 'ellipsis2', 10] for totalPages=10, currentPage=5 */
36-61: Extract magic numbers into named constants.The function uses several magic numbers that should be constants for better maintainability and clarity.
+const MAX_VISIBLE_PAGES = 7; +const ELLIPSIS_THRESHOLD = 3; +const VISIBLE_PAGES_AROUND_CURRENT = 1; const getPageNumbers = (): (number | string)[] => { - if (totalPages <= 7) { + if (totalPages <= MAX_VISIBLE_PAGES) { return Array.from({ length: totalPages }, (_, i) => i + 1); } const pages: (number | string)[] = [1]; - if (currentPage > 3) { + if (currentPage > ELLIPSIS_THRESHOLD) { pages.push('ellipsis1'); } - const start = Math.max(2, currentPage - 1); - const end = Math.min(totalPages - 1, currentPage + 1); + const start = Math.max(2, currentPage - VISIBLE_PAGES_AROUND_CURRENT); + const end = Math.min(totalPages - 1, currentPage + VISIBLE_PAGES_AROUND_CURRENT); for (let i = start; i <= end; i++) { pages.push(i); } - if (currentPage < totalPages - 2) { + if (currentPage < totalPages - ELLIPSIS_THRESHOLD + 1) { pages.push('ellipsis2'); } pages.push(totalPages); return pages; };
69-71: Extract common button styles into Tailwind components.The button styles are duplicated between Previous and Next buttons. Consider extracting them into Tailwind components.
Add these classes to your Tailwind CSS file:
@layer components { .pagination-button { @apply font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em]; } .pagination-button-enabled { @apply text-[#141717] hover:bg-gray-50; } .pagination-button-disabled { @apply cursor-not-allowed text-gray-300 hover:bg-gray-white; } }Then simplify the button classes:
- className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ - currentPage === totalPages - ? 'cursor-not-allowedtext-gray-300 hover:bg-gray-white text-gray-300' - : 'text-[#141717] hover:bg-gray-50' - }`} + className={`pagination-button ${ + currentPage === totalPages + ? 'pagination-button-disabled' + : 'pagination-button-enabled' + }`}Also applies to: 106-110
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/icons/Previous.tsx(1 hunks)components/pagination/Pagination.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/icons/Previous.tsx
components/pagination/Pagination.tsx
Outdated
| currentPage === totalPages | ||
| ? 'cursor-not-allowedtext-gray-300 hover:bg-gray-white text-gray-300' | ||
| : 'text-[#141717] hover:bg-gray-50' |
There was a problem hiding this comment.
Fix CSS class concatenation in Next button's disabled state.
There's a typo in the class concatenation that might affect styling.
- ? 'cursor-not-allowedtext-gray-300 hover:bg-gray-white text-gray-300'
+ ? 'cursor-not-allowed text-gray-300 hover:bg-gray-white'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| currentPage === totalPages | |
| ? 'cursor-not-allowedtext-gray-300 hover:bg-gray-white text-gray-300' | |
| : 'text-[#141717] hover:bg-gray-50' | |
| currentPage === totalPages | |
| ? 'cursor-not-allowed text-gray-300 hover:bg-gray-white' | |
| : 'text-[#141717] hover:bg-gray-50' |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/pagination/Pagination.tsx (4)
11-12: Remove unnecessary eslint-disable comment.The empty eslint-disable comment can be safely removed as it serves no purpose.
- // eslint-disable-next-line prettier/prettier -
27-31: Add input validation for page numbers.While the bounds checking is good, consider adding type validation and handling edge cases.
const handlePageChange = (page: number) => { + // Handle NaN and non-integer values + if (!Number.isInteger(page)) { + console.warn('Invalid page number:', page); + return; + } if (page >= 1 && page <= totalPages) { onPageChange(page); } };
36-61: Improve code maintainability with constants and better documentation.The pagination logic could be more maintainable with named constants and improved documentation.
+ // Constants for pagination configuration + const MAX_VISIBLE_PAGES = 7; + const ELLIPSIS_START = 'ellipsis_start'; + const ELLIPSIS_END = 'ellipsis_end'; + const getPageNumbers = (): (number | string)[] => { - if (totalPages <= 7) { + // Show all pages if total pages is less than or equal to maximum visible pages + if (totalPages <= MAX_VISIBLE_PAGES) { return Array.from({ length: totalPages }, (_, i) => i + 1); } const pages: (number | string)[] = [1]; if (currentPage > 3) { - pages.push('ellipsis1'); + pages.push(ELLIPSIS_START); } + // Calculate the range of visible page numbers around the current page const start = Math.max(2, currentPage - 1); const end = Math.min(totalPages - 1, currentPage + 1); for (let i = start; i <= end; i++) { pages.push(i); } if (currentPage < totalPages - 2) { - pages.push('ellipsis2'); + pages.push(ELLIPSIS_END); } pages.push(totalPages); return pages; };
63-115: Optimize button styles and reduce duplication.The Previous and Next button styles are duplicated. Consider extracting common styles and simplifying the conditional classes.
+ // Extract common button styles + const getButtonStyles = (isDisabled: boolean) => ` + font-normal flex h-[34px] items-center justify-center + rounded bg-white px-3 py-[7px] text-sm leading-[17px] + tracking-[-0.01em] ${ + isDisabled + ? 'hover:bg-gray-white cursor-not-allowed text-gray-300' + : 'text-[#141717] hover:bg-gray-50' + } + `; return ( <nav role='navigation' aria-label='Pagination' className='font-inter flex items-center justify-center gap-8'> {/* Previous button */} <Button onClick={() => handlePageChange(currentPage - 1)} disabled={currentPage === 1} - className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ - currentPage === 1 ? 'hover:bg-gray-white cursor-not-allowed text-gray-300' : 'text-[#141717] hover:bg-gray-50' - }`} + className={getButtonStyles(currentPage === 1)} text='Previous' icon={<IconPrevious />} iconPosition={ButtonIconPosition.LEFT} aria-label='Go to previous page' /> {/* ... page numbers ... */} {/* Next button */} <Button onClick={() => handlePageChange(currentPage + 1)} disabled={currentPage === totalPages} - className={`font-normal flex h-[34px] items-center justify-center rounded bg-white px-3 py-[7px] text-sm leading-[17px] tracking-[-0.01em] ${ - currentPage === totalPages - ? 'hover:bg-gray-white cursor-not-allowed text-gray-300' - : 'text-[#141717] hover:bg-gray-50' - }`} + className={getButtonStyles(currentPage === totalPages)} text='Next' icon={<IconNext />} aria-label='Go to next page' /> </nav> );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/pagination/Pagination.tsx(1 hunks)
🔇 Additional comments (1)
components/pagination/Pagination.tsx (1)
26-26: Add prop validation for edge cases.Consider adding validation for the component props to handle edge cases gracefully.
export default function Pagination({ totalPages, currentPage, onPageChange }: PaginationProps) { + // Validate props + if (totalPages < 1) { + console.warn('totalPages must be greater than 0'); + return null; + } + if (currentPage < 1 || currentPage > totalPages) { + console.warn('currentPage must be between 1 and totalPages'); + return null; + }
Description
Add pagination feature in Blog page
Related issue(s)
Fixes #3570
update.mp4
Related issue(s)
Summary by CodeRabbit
Release Notes
New Features
PaginationItemcomponent for individual page numbers.Improvements