fix: prevent disabled pagination buttons from triggering navigation - #18
fix: prevent disabled pagination buttons from triggering navigation#18ashishSoni1234 wants to merge 3 commits into
Conversation
The shared PaginationLink renders an <a> tag, so the native `disabled` attribute/CSS pseudo-class never applied, and CustomPagination only set aria-disabled (a screen-reader hint) while leaving onClick unguarded - clicking Previous on page 1 or Next on the last page still pushed an out-of-range page via router.push. PaginationLink now accepts a `disabled` prop that guards onClick, removes the element from tab order (tabIndex=-1), and sets aria-disabled for assistive tech. CustomPagination passes disabled based on the current page and clamps createPageURL as a defense-in-depth backstop. Adds Vitest + React Testing Library (previously no test setup existed) with regression tests for both the shared primitive and the consumer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@ashishSoni1234 is attempting to deploy a commit to the MAY55A's projects Team on Vercel. A member of the Team first needs to authorize it. |
MAY55A
left a comment
There was a problem hiding this comment.
@ashishSoni1234, thank you for your effort!
However, adding tests and setting up a test framework is out of scope for this issue.
There is another open issue (#12) addressing tests configuration, you can work on that one if you'd like, then you can come back to this issue and proceed with the pagination tests.
But for now, it would be more helpful to just fix the pagination issue in the already existing files.
Maintainer feedback on MAY55A#18: adding tests and a test framework is out of scope for this issue - that belongs to MAY55A#12. Reverting package.json/package-lock.json/tsconfig.json and removing the vitest config, setup file, and *.test.tsx files added in the previous commit. The pagination fix itself is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hi @MAY55A the test framework and test files have been removed in the |
MAY55A
left a comment
There was a problem hiding this comment.
@ashishSoni1234 , thank you for removing the test setup part !
The pagination issue seems to be almost fixed, but there is this case that triggers a problem, you can reproduce it this way:
- add manually page=2 as a URL param (in discussions page for example)
- the page shows no items (no discussions for example)
previousbutton is enabled and clickable- however, when clicking
previousno action is triggered
The case is not only for a page value of
2but for all values greater thantotalPagesor less than0
The problem is that the UI of the button shows something different from its actual behavior (looks clickable but triggers no action), it should reflect exactly what it does.
To address this problem from its root, try fixing the out-of-range problem (when currentPage < 1 or currentPage > totalPages) by doing this:
- make sure that
currentPageis always a valid value (between1andtotalPages) - change the out-of-range page value in the URL param to always show a valid value (rewrite the URL param without redirecting when necessary)
- make sure no UI shows a non-valid value
I believe these changes will make the pagination mechanism more robust and prevent any unwanted behaviors in the future.
If you notice any other unexpected behaviors, don't hesitate to address them as well !
Reviewer feedback on MAY55A#18: setting ?page to a value outside [1, totalPages] (e.g. page=2 when there's only 1 page, or a negative/huge value) left the UI and behavior mismatched - Previous/Next disabled state was computed from the raw, unclamped page, so a button could render as enabled while its click handler silently no-opped (createPageURL's own bounds check rejected the target page). currentPage is now derived by clamping the raw URL value into [1, totalPages] once, and every piece of UI (displayed page number, Previous/Next disabled state, click targets) reads from that single clamped value, so what's rendered always matches what clicking it does. An effect rewrites the URL to the clamped value via router.replace when they diverge, so an invalid ?page never lingers after the first render. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@MAY55A thanks for catching that! Pushed a fix — Verified manually with page=2/0/-5/very-large values against 1-page and |
MAY55A
left a comment
There was a problem hiding this comment.
@ashishSoni1234, thank you for your work !
The issue seems to be fixed, when testing on the discussions page.
However, on the projects page, there seems to be a problem, though it seems like a server/DB side issue, but it does reflect on the front and prevents the display of the pagination buttons.
It would be of great help, if you could investigate the problem further, try starting with console logs to see what's causing it.
Thank you greatly for you dedication and hard work !
|
|
||
| const createPageURL = (pageNumber: number | string) => { | ||
| const safeTotalPages = Math.max(totalPages, 1); | ||
| const rawPage = Number(searchParams.get('page')) || 1; |
There was a problem hiding this comment.
Use ?? instead of || to make sure rawPage can keep its original 0 value, so the URL param can be changed and updated to 1 (currently ?page=0 is allowed in the URL)
The shared PaginationLink renders an tag, so the native
disabledattribute/CSS pseudo-class never applied, and CustomPagination only set aria-disabled (a screen-reader hint) while leaving onClick unguarded - clicking Previous on page 1 or Next on the last page still pushed an out-of-range page via router.push.PaginationLink now accepts a
disabledprop that guards onClick, removes the element from tab order (tabIndex=-1), and sets aria-disabled for assistive tech. CustomPagination passes disabled based on the current page and clamps createPageURL as a defense-in-depth backstop.Adds Vitest + React Testing Library (previously no test setup existed) with regression tests for both the shared primitive and the consumer.