bugfix/pin 9329 elements per page visible when table is empty#84
bugfix/pin 9329 elements per page visible when table is empty#84martinaCampoli wants to merge 5 commits intodevelopfrom
Conversation
martinaCampoli
requested review from
Alepazz,
Carminepo2,
borgesis95 and
ruggerocastagnola
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request addresses PIN-9329 and PIN-9341 by updating the default rows-per-page options from [10, 24, 36] to [12, 24, 36] and hiding the rows-per-page selector when tables are empty (totalPages = 0). The changes aim to improve the pagination UX by preventing the display of pagination controls when there's no data to paginate.
Changes:
- Updated default rows-per-page options from [10, 24, 36] to [12, 24, 36] in the Pagination component
- Added condition to hide rows-per-page selector when totalPages is 0
- Updated test expectations to reflect the new default options [12, 24, 36]
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/features/pagination/components/Pagination.tsx | Updated defaultOptions constant to [12, 24, 36] and added totalPages > 0 condition to conditionally render the rows-per-page selector |
| src/features/pagination/tests/Pagination.test.tsx | Updated test description and assertions to match new default options [12, 24, 36] |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {...stackProps} | ||
| > | ||
| {rowPerPageOptions && ( | ||
| {rowPerPageOptions && totalPages > 0 && ( |
There was a problem hiding this comment.
The condition added here (totalPages > 0) successfully hides the rows-per-page selector when the table is empty. However, there's a layout consideration: the parent Stack's justifyContent is set to 'space-between' when rowPerPageOptions is provided (line 55, which is outside the diff), but now the selector won't render when totalPages is 0. If both the selector (totalPages = 0) and the pagination controls (totalPages <= 1) don't render, you'll have an empty Stack with 'space-between' justification. While this doesn't break functionality, it may be worth ensuring the justifyContent logic aligns with the actual rendering conditions.
Alepazz
left a comment
Alepazz
left a comment
There was a problem hiding this comment.
I totally agree with Copilot review.
I think that there isn't the test for totalPages == 0 case scenario. Could you verify?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tionExampleWithoutTotalPages
Issue
PIN-9329
PIN-9341
Description / Context / Why
This pull request updates the default rows-per-page options in the pagination component from [10, 24, 36] to [12, 24, 36], and ensures that the rows-per-page selector is only shown when there are pages to display. Related tests have also been updated to reflect the new default options.