feat: add pagination feature in blog page #3595
Conversation
pryxnsu
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
magicmatatjahu
as code owners
WalkthroughAdds a pagination hook and UI components, integrates pagination into the blog index with router query syncing, adds next/previous SVG icons, updates filter to ignore the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Blog as BlogIndexPage
participant Hook as usePagination
participant Router as Next.js Router
participant UI as Pagination
User->>Blog: open /blog?page=n
Blog->>Hook: init(items, itemsPerPage=9)
Hook-->>Blog: { currentPage, currentItems, maxPage }
Blog->>UI: render(currentPage, totalPages=maxPage)
alt User clicks page/prev/next
User->>UI: click
UI-->>Blog: onPageChange(page)
Blog->>Hook: setCurrentPage(page)
Blog->>Router: replace({ query: {..., page} })
Hook-->>Blog: updated currentItems
Blog-->>User: re-render with new items
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 project 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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3595 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 798 798
Branches 146 146
=========================================
Hits 798 798 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⚡️ 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
There was a problem hiding this comment.
@priyanshuxkumar Update the name of the type. Type name as T is way too generic.
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 -
…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 />} |
| 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.
@priyanshuxkumar Please add these aria-label for good accessibility in the pages.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@akshatnema could you please review this and let me know if any further changes are required? |
TRohit20
left a comment
TRohit20
left a comment
There was a problem hiding this comment.
Hi @priyanshuxkumar Kindly resolve the conversations with relevant fixes and suggestions made by CodeRabbit if relevant :)
components/buttons/Button.tsx
Outdated
|
|
||
| /** The text to be displayed on the button. */ | ||
| text: string; | ||
| text: string | React.ReactNode; |
There was a problem hiding this comment.
This prop field is for text string only. Why do you need to add React component here?
components/helpers/applyFilter.ts
Outdated
|
|
||
| if (query && Object.keys(query).length >= 1) { | ||
| Object.keys(query).forEach((property) => { | ||
| if (nonFilterableKeys.includes(property)) { |
There was a problem hiding this comment.
I don't understand what is the need of nonFilterable Key field here. If you don't want to add it in filtering, simply don't pass it in criteria or input data.
There was a problem hiding this comment.
ok , i have updated it
// Filter.tsx
useEffect(() => {
const filterableQuery = Object.fromEntries(Object.entries(routeQuery).filter(([key]) => key !== 'page'));
onFilterApply(data, onFilter, filterableQuery);
}, [routeQuery]);
| /** | ||
| * @description Icons for Next button | ||
| */ | ||
| export default function IconNext() { |
There was a problem hiding this comment.
Why there is a need of having a separate component for these icons? Can't we use these images in SVG format?
There was a problem hiding this comment.
We can, but components with SVG images already exist. I did the same
pryxnsu
force-pushed
the
feat-pagination
branch
from
4b1bc70 to
f79b659
Compare
|
Hi @sambhavgupta0705 @akshatnema, I’ve fixed the issues and updated the approach. Would appreciate your review. Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
components/buttons/Button.tsx (1)
78-88: Avoid spacing regression between icon and text by adding conditional marginsRemoving the mr-2/ml-2 created zero gap between the icon and label for all existing button usages that render both. To keep pagination’s icon-only buttons tight while preserving spacing elsewhere, add margins only when text is non-empty.
Apply this diff:
- {icon && iconPosition === ButtonIconPosition.LEFT && ( - <span className='inline-block' data-testid='Button-icon-left'> + {icon && iconPosition === ButtonIconPosition.LEFT && ( + <span + className={text?.trim() ? 'mr-2 inline-block' : 'inline-block'} + data-testid='Button-icon-left' + > {icon} </span> )} <span className='inline-block'>{text}</span> - {icon && iconPosition === ButtonIconPosition.RIGHT && ( - <span className='inline-block' data-testid='Button-icon-right'> + {icon && iconPosition === ButtonIconPosition.RIGHT && ( + <span + className={text?.trim() ? 'ml-2 inline-block' : 'inline-block'} + data-testid='Button-icon-right' + > {icon} </span> )} @@ - {icon && iconPosition === ButtonIconPosition.LEFT && <span className='inline-block'>{icon}</span>} + {icon && iconPosition === ButtonIconPosition.LEFT && ( + <span className={text?.trim() ? 'mr-2 inline-block' : 'inline-block'}>{icon}</span> + )} <span className='inline-block'>{text}</span> - {icon && iconPosition === ButtonIconPosition.RIGHT && <span className='inline-block'>{icon}</span>} + {icon && iconPosition === ButtonIconPosition.RIGHT && ( + <span className={text?.trim() ? 'ml-2 inline-block' : 'inline-block'}>{icon}</span> + )}Follow-up:
- Please sanity-check a few existing buttons in the preview (primary/secondary, with left/right icons) to ensure spacing matches design expectations.
Also applies to: 101-104
components/navigation/Filter.tsx (2)
63-76: Reset pagination when filters change to avoid empty pagesIf a user is on page N and applies a filter that reduces the result set, N may become out of range, producing an empty list until they manually go back. Clear the page param when any filter changes so pagination resets to page 1.
Apply this diff:
onChange={(e) => { const { query } = route; const newQuery = { ...query }; if (e) { newQuery[check.name] = e; } else { // Remove a specific filter upon clicking Select Placeholder option delete newQuery[check.name]; } + // Reset pagination whenever filters change to avoid out-of-range pages + delete newQuery.page; if (newQuery) { const queryParams = new URLSearchParams(newQuery as { [key: string]: string }).toString();Optional: also consider clearing page in bulk filter changes (if any) elsewhere in the app for consistency.
32-35: Tighten the dependency and destructuring for readabilityThe effect depends on route.query content, not the router instance. Depending on the router object may re-run more often than needed. Also, simple destructuring reads better.
-useEffect(() => { - setQuery(route.query); - applyFilterList(checks, data, setFilters); -}, [route]); +useEffect(() => { + setQuery(route.query); + applyFilterList(checks, data, setFilters); +}, [route.query]);And if you prefer, the filterableQuery can be written as:
-const filterableQuery = Object.fromEntries(Object.entries(routeQuery).filter(([key]) => key !== 'page')); +const { page: _omit, ...filterableQuery } = routeQuery;components/pagination/Pagination.tsx (5)
27-31: Avoid no-op state updates by ignoring clicks on the active pageGuard against redundant onPageChange calls that can trigger unnecessary rerenders and router pushes.
- const handlePageChange = (page: number) => { - if (page >= 1 && page <= totalPages) { - onPageChange(page); - } - }; + const handlePageChange = (page: number) => { + if (page !== currentPage && page >= 1 && page <= totalPages) { + onPageChange(page); + } + };
94-115: Role mismatch: container uses role="list" but children are buttons, not listitemsEither use proper list semantics (ul/li) with role="listitem" or drop role="list" here. Given we already have a nav landmark, simplest is to remove the list role.
- <div className='flex gap-2' role='list'> + <div className='flex gap-2'>
102-103: Redundant aria-current prop; PaginationItem sets it internallyPaginationItem already sets aria-current based on isActive. Passing it again is unnecessary and can drift if implementations diverge.
- aria-current={page === currentPage ? 'page' : undefined}
80-86: Verify custom Tailwind class 'hover:bg-gray-white' or replace with a safe default'hover:bg-gray-white' is not a standard Tailwind color. If not defined in your Tailwind config, the class will be ignored. Consider using disabled: variants and a concrete color.
Option A (verify exists): keep as-is if configured.
Option B (safer, Tailwind-native):
- currentPage === 1 - ? 'hover:bg-gray-white cursor-not-allowed text-gray-300' - : 'text-[#141717] hover:bg-gray-50' + 'disabled:cursor-not-allowed disabled:text-gray-300 text-[#141717] hover:bg-gray-50'and rely on the disabled prop already set on the Button.
Also applies to: 121-127
70-76: Consider short-circuiting render when totalPages <= 1This component may be reused elsewhere beyond Blog, where the caller might not guard. Returning null when there’s nothing to paginate avoids extra DOM.
Example:
if (totalPages <= 1) return null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/buttons/Button.tsx(2 hunks)components/navigation/Filter.tsx(1 hunks)components/pagination/Pagination.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
PR: asyncapi/website#3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/pagination/Pagination.tsx
🧬 Code graph analysis (2)
components/navigation/Filter.tsx (1)
components/helpers/applyFilter.ts (1)
onFilterApply(131-159)
components/pagination/Pagination.tsx (4)
components/buttons/Button.tsx (1)
Button(53-106)components/icons/Previous.tsx (1)
IconPrevious(7-20)components/pagination/PaginationItem.tsx (1)
PaginationItem(19-32)components/icons/Next.tsx (1)
IconNext(7-20)
🔇 Additional comments (1)
components/navigation/Filter.tsx (1)
38-41: Good call excluding the page query from filteringFiltering with routeQuery minus page prevents page changes from retriggering or narrowing results unintentionally. This aligns well with pagination UX.
|
@priyanshuxkumar please look into codeRabbitAI suggestions |
i have made the changes. please have a look |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/pagination/Pagination.tsx (5)
78-100: Use semantic list markup instead of role='list'.Prefer
<ul><li>for pagination lists. This improves a11y and reduces ARIA misuse.- {/* Page numbers */} - <div className='flex gap-2' role='list'> + {/* Page numbers */} + <ul className='flex gap-2'> {getPageNumber().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} - /> + <li key={page} className='list-none'> + <PaginationItem + pageNumber={page} + isActive={page === currentPage} + onPageChange={handlePageChange} + /> + </li> ) : ( - <span - key={page} - className='font-inter flex size-10 items-center justify-center text-sm font-semibold text-[#6B6B6B]' - aria-hidden='true' - > - ... - </span> + <li key={page} className='list-none' aria-hidden='true'> + <span className='font-inter flex size-10 items-center justify-center text-sm font-semibold text-[#6B6B6B]'> + ... + </span> + </li> ) )} - </div> + </ul>
65-71: Remove non-existent Tailwind class 'hover:bg-gray-white'.Tailwind doesn’t ship
bg-gray-white. Drop it to avoid dead classes.- ? 'hover:bg-gray-white cursor-not-allowed text-gray-300' + ? 'cursor-not-allowed text-gray-300'Apply in both Prev and Next button conditionals.
Also applies to: 106-112
.
56-59: Redundant role on
<nav>already implies the navigation role. Keep onlyaria-label.- <nav - role='navigation' - aria-label='Pagination' - className='font-inter flex min-w-[326px] items-center justify-center md:gap-8' - > + <nav aria-label='Pagination' className='font-inter flex min-w-[326px] items-center justify-center md:gap-8'>
11-12: Remove stray prettier disable comment.This no-op directive adds noise.
- // eslint-disable-next-line prettier/prettier -
73-75: Mark inline SVG icons as decorative.Since buttons have
aria-label, hide SVGs from AT to prevent duplicate announcements.Update components/icons/Previous.tsx and components/icons/Next.tsx:
- <svg + <svg + aria-hidden='true' + focusable='false' width='20' height='20' viewBox='0 0 24 24' fill='none' xmlns='http://www.w3.org/2000/svg' className='stroke-current'Also applies to: 114-115
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/pagination/Pagination.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
PR: asyncapi/website#3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/pagination/Pagination.tsx
🧬 Code graph analysis (1)
components/pagination/Pagination.tsx (4)
components/buttons/Button.tsx (1)
Button(53-106)components/icons/Previous.tsx (1)
IconPrevious(7-20)components/pagination/PaginationItem.tsx (1)
PaginationItem(19-32)components/icons/Next.tsx (1)
IconNext(7-20)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/pagination/PaginationItem.tsx (3)
26-36: Improve a11y: explicit label and visible focus styles.Add an aria-label for screen readers and a clear focus ring for keyboard users.
- <button + <button {...rest} type="button" onClick={handleClick} - className={`font-inter font-normal relative flex size-10 items-center justify-center rounded-full text-sm leading-[26px] ${isActive ? 'bg-[#6200EE] text-white' : 'bg-transparent text-[#141717] hover:bg-gray-50'} ${className ?? ''}`} + className={`font-inter font-normal relative flex size-10 items-center justify-center rounded-full text-sm leading-[26px] ${isActive ? 'bg-[#6200EE] text-white' : 'bg-transparent text-[#141717] hover:bg-gray-50'} focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[#6200EE] focus-visible:ring-offset-2 ${className ?? ''}`} aria-current={isActive ? 'page' : undefined} + aria-label={isActive ? \`Current page, \${pageNumber}\` : \`Go to page \${pageNumber}\`} >
4-5: Remove stray Prettier disable comment.This suppression doesn’t guard any specific line and should be dropped.
- // eslint-disable-next-line prettier/prettier -
28-31: Avoid hard-coded hex colors; prefer theme tokens.Using Tailwind palette (e.g.,
bg-primary-600,text-foreground,text-gray-900 dark:text-gray-100) or CSS vars keeps dark mode and theming consistent across the site.If the project exposes a primary color token, swap
bg-[#6200EE]/text-[#141717]for those tokens to match design system theming.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/pagination/Pagination.tsx(1 hunks)components/pagination/PaginationItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/pagination/Pagination.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
| export default function PaginationItem({ | ||
| pageNumber, | ||
| isActive, | ||
| onPageChange, | ||
| ...buttonProps | ||
| }: PaginationItemProps & React.ButtonHTMLAttributes<HTMLButtonElement>) { | ||
| return ( | ||
| <button | ||
| onClick={() => onPageChange(pageNumber)} | ||
| className={`font-inter font-normal relative flex size-10 items-center | ||
| justify-center rounded-full text-sm leading-[26px] | ||
| ${isActive ? 'bg-[#6200EE] text-white' : 'bg-transparent text-[#141717] hover:bg-gray-50'} | ||
| `} | ||
| aria-current={isActive ? 'page' : undefined} | ||
| {...buttonProps} | ||
| > | ||
| {pageNumber} | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent prop overrides: merge onClick, lock aria-current, and add type="button".
As written, spreading buttonProps after your props allows consumers to override onClick (breaking pagination) and aria-current. Also, without type="button", this can submit a surrounding form. Merge handlers, place the spread first, and set type="button".
-export default function PaginationItem({
- pageNumber,
- isActive,
- onPageChange,
- ...buttonProps
-}: PaginationItemProps & React.ButtonHTMLAttributes<HTMLButtonElement>) {
- return (
- <button
- onClick={() => onPageChange(pageNumber)}
- className={`font-inter font-normal relative flex size-10 items-center
- justify-center rounded-full text-sm leading-[26px]
- ${isActive ? 'bg-[#6200EE] text-white' : 'bg-transparent text-[#141717] hover:bg-gray-50'}
- `}
- aria-current={isActive ? 'page' : undefined}
- {...buttonProps}
- >
- {pageNumber}
- </button>
- );
+export default function PaginationItem(
+ {
+ pageNumber,
+ isActive,
+ onPageChange,
+ onClick,
+ className,
+ ...rest
+ }: PaginationItemProps & React.ButtonHTMLAttributes<HTMLButtonElement>
+) {
+ const handleClick = (e: React.MouseEvent<HTMLButtonElement>) => {
+ onClick?.(e);
+ if (!e.defaultPrevented) onPageChange(pageNumber);
+ };
+ return (
+ <button
+ {...rest}
+ type="button"
+ onClick={handleClick}
+ className={`font-inter font-normal relative flex size-10 items-center justify-center rounded-full text-sm leading-[26px] ${isActive ? 'bg-[#6200EE] text-white' : 'bg-transparent text-[#141717] hover:bg-gray-50'} ${className ?? ''}`}
+ aria-current={isActive ? 'page' : undefined}
+ >
+ {pageNumber}
+ </button>
+ );📝 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 PaginationItem({ | |
| pageNumber, | |
| isActive, | |
| onPageChange, | |
| ...buttonProps | |
| }: PaginationItemProps & React.ButtonHTMLAttributes<HTMLButtonElement>) { | |
| return ( | |
| <button | |
| onClick={() => onPageChange(pageNumber)} | |
| className={`font-inter font-normal relative flex size-10 items-center | |
| justify-center rounded-full text-sm leading-[26px] | |
| ${isActive ? 'bg-[#6200EE] text-white' : 'bg-transparent text-[#141717] hover:bg-gray-50'} | |
| `} | |
| aria-current={isActive ? 'page' : undefined} | |
| {...buttonProps} | |
| > | |
| {pageNumber} | |
| </button> | |
| export default function PaginationItem( | |
| { | |
| pageNumber, | |
| isActive, | |
| onPageChange, | |
| onClick, | |
| className, | |
| ...rest | |
| }: PaginationItemProps & React.ButtonHTMLAttributes<HTMLButtonElement> | |
| ) { | |
| const handleClick = (e: React.MouseEvent<HTMLButtonElement>) => { | |
| onClick?.(e); | |
| if (!e.defaultPrevented) onPageChange(pageNumber); | |
| }; | |
| return ( | |
| <button | |
| {...rest} | |
| type="button" | |
| onClick={handleClick} | |
| className={`font-inter font-normal relative flex size-10 items-center justify-center rounded-full text-sm leading-[26px] ${ | |
| isActive | |
| ? 'bg-[#6200EE] text-white' | |
| : 'bg-transparent text-[#141717] hover:bg-gray-50' | |
| } ${className ?? ''}`} | |
| aria-current={isActive ? 'page' : undefined} | |
| > | |
| {pageNumber} | |
| </button> | |
| ); | |
| } |
🤖 Prompt for AI Agents
components/pagination/PaginationItem.tsx lines 19-36: spread of buttonProps
currently comes after component props allowing consumers to override onClick and
aria-current, and missing type attribute can submit parent forms; fix by
spreading buttonProps first, add type="button", and create a merged onClick
handler that calls the component's onPageChange(pageNumber) then calls
buttonProps.onClick if present (preserve event propagation and prevent default
handling only if callers rely on it), and set aria-current explicitly based on
isActive after the spread so it cannot be overridden.
6 participants
Description
Add pagination feature in Blog page
Related issue(s)
Fixes #3570
update.mp4
Related issue(s)
Summary by CodeRabbit